[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Frank Li]

When added above configuration, iram fix up plus relocate offset may locate
in invalidate space. Write back fix up value will cause data abort.

Add address check, skip psci code.

Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 arch/arm/lib/relocate.S | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
index 475d503..6795a1b 100644
--- a/arch/arm/lib/relocate.S
+++ b/arch/arm/lib/relocate.S
@@ -99,6 +99,10 @@ fixloop:
 	cmp	r1, #23			/* relative fixup? */
 	bne	fixnext
 
+	ldr	r1, =__image_copy_start
+	cmp	r0, r1
+	blo	fixnext
+
 	/* relative fix: increase location by offset */
 	add	r0, r0, r4
 	ldr	r1, [r0]
-- 
2.5.2

[U-Boot] [PATCH 2/3] mx7: psci: add basic psci support

[Frank Li]

add basic psci support for imx7 chip.
support cpu_on and cpu_off.
linux kernel boot at nosecure mode.
set csu allow nosecure mode kernel to access all peripherial register

Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 arch/arm/cpu/armv7/mx7/Makefile   |  4 ++
 arch/arm/cpu/armv7/mx7/psci-mx7.c | 79 +++++++++++++++++++++++++++++++++++++++
 arch/arm/cpu/armv7/mx7/psci.S     | 54 ++++++++++++++++++++++++++
 arch/arm/cpu/armv7/mx7/soc.c      |  9 +++++
 4 files changed, 146 insertions(+)
 create mode 100644 arch/arm/cpu/armv7/mx7/psci-mx7.c
 create mode 100644 arch/arm/cpu/armv7/mx7/psci.S

diff --git a/arch/arm/cpu/armv7/mx7/Makefile b/arch/arm/cpu/armv7/mx7/Makefile
index e6ecef0..f25461c 100644
--- a/arch/arm/cpu/armv7/mx7/Makefile
+++ b/arch/arm/cpu/armv7/mx7/Makefile
@@ -6,3 +6,7 @@
 #
 
 obj-y	:= soc.o clock.o clock_slice.o
+
+ifdef CONFIG_ARMV7_PSCI
+obj-y  += psci.o psci-mx7.o
+endif
diff --git a/arch/arm/cpu/armv7/mx7/psci-mx7.c b/arch/arm/cpu/armv7/mx7/psci-mx7.c
new file mode 100644
index 0000000..6659332
--- /dev/null
+++ b/arch/arm/cpu/armv7/mx7/psci-mx7.c
@@ -0,0 +1,79 @@
+#include <common.h>
+#include <asm/psci.h>
+#include <asm/arch/imx-regs.h>
+
+#define __secure __attribute__ ((section ("._secure.text")))
+
+#define GPC_CPU_PGC_SW_PDN_REQ	0xfc
+#define GPC_CPU_PGC_SW_PUP_REQ	0xf0
+#define GPC_PGC_C1		0x840
+
+#define BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7	0x2
+
+/* below is for i.MX7D */
+#define SRC_GPR1_MX7D		0x074
+#define SRC_A7RCR0		0x004
+#define SRC_A7RCR1		0x008
+
+#define BP_SRC_A7RCR0_A7_CORE_RESET0	0
+#define BP_SRC_A7RCR1_A7_CORE1_ENABLE	1
+
+static inline void psci_writel(u32 value, u32 reg)
+{
+	*(volatile u32 *)reg = value;
+}
+
+static inline int psci_readl(u32 reg)
+{
+	return *(volatile u32*)reg;
+}
+
+static inline void imx_gpcv2_set_m_core_pgc(bool enable, u32 offset)
+{
+	psci_writel(enable, GPC_IPS_BASE_ADDR + offset);
+}
+
+__secure void imx_gpcv2_set_core1_power(bool pdn)
+{
+	u32 reg = pdn ? GPC_CPU_PGC_SW_PUP_REQ : GPC_CPU_PGC_SW_PDN_REQ;
+	u32 val;
+
+	imx_gpcv2_set_m_core_pgc(true, GPC_PGC_C1);
+
+	val = psci_readl(GPC_IPS_BASE_ADDR + reg);
+	val |= BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7;
+	psci_writel(val, GPC_IPS_BASE_ADDR + reg);
+
+	while ((psci_readl(GPC_IPS_BASE_ADDR + reg) & BM_CPU_PGC_SW_PDN_PUP_REQ_CORE1_A7) != 0)
+		;
+
+	imx_gpcv2_set_m_core_pgc(false, GPC_PGC_C1);
+}
+
+__secure void imx_enable_cpu_ca7(int cpu, bool enable)
+{
+	u32 mask, val;
+
+	mask = 1 << (BP_SRC_A7RCR1_A7_CORE1_ENABLE + cpu - 1);
+	val = psci_readl(SRC_BASE_ADDR + SRC_A7RCR1);
+	val = enable ? val | mask : val & ~mask;
+	psci_writel(val, SRC_BASE_ADDR + SRC_A7RCR1);
+}
+
+
+__secure int imx_cpu_on(int fn, int cpu, int pc)
+{
+	psci_writel(pc, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D);
+	imx_gpcv2_set_core1_power(true);
+	imx_enable_cpu_ca7(cpu, true);
+	return 0;
+}
+
+__secure int imx_cpu_off(int cpu)
+{
+	imx_enable_cpu_ca7(cpu, false);
+	imx_gpcv2_set_core1_power(false);
+	psci_writel(0, SRC_BASE_ADDR + cpu * 8 + SRC_GPR1_MX7D + 4);
+	return 0;
+}
+
diff --git a/arch/arm/cpu/armv7/mx7/psci.S b/arch/arm/cpu/armv7/mx7/psci.S
new file mode 100644
index 0000000..4165cf5
--- /dev/null
+++ b/arch/arm/cpu/armv7/mx7/psci.S
@@ -0,0 +1,54 @@
+#include <config.h>
+#include <linux/linkage.h>
+
+#include <asm/armv7.h>
+#include <asm/arch-armv7/generictimer.h>
+#include <asm/psci.h>
+
+	.pushsection ._secure.text, "ax"
+
+	.arch_extension sec
+
+	@ r1 = target CPU
+	@ r2 = target PC
+
+.globl	psci_arch_init
+psci_arch_init:
+	mov	r6, lr
+
+	bl	psci_get_cpu_id
+	bl	psci_get_cpu_stack_top
+	mov	sp, r0
+
+	bx	r6
+
+	@ r1 = target CPU
+	@ r2 = target PC
+
+.globl psci_cpu_on
+psci_cpu_on:
+	push {lr}
+
+	mov r0, r1
+	bl psci_get_cpu_stack_top
+	str r2, [r0]
+	dsb
+
+	ldr	r2, =psci_cpu_entry
+	bl imx_cpu_on
+
+	pop	{pc}
+
+.globl psci_cpu_off
+psci_cpu_off:
+
+	bl psci_cpu_off_common
+	bl psci_get_cpu_id
+	bl imx_cpu_off
+
+1: 	wfi
+	b 1b
+
+	.globl psci_text_end
+psci_text_end:
+	.popsection
diff --git a/arch/arm/cpu/armv7/mx7/soc.c b/arch/arm/cpu/armv7/mx7/soc.c
index 8d50149..cfe2b1c 100644
--- a/arch/arm/cpu/armv7/mx7/soc.c
+++ b/arch/arm/cpu/armv7/mx7/soc.c
@@ -116,10 +116,19 @@ u32 __weak get_board_rev(void)
 }
 #endif
 
+/* enable all periherial can be access in nosec mode */
+static void init_csu(void)
+{
+	int i = 0;
+	for (i = 0; i < 64; i++)
+		writel(0x00FF00FF, 0x303e0000 + i * 4);
+}
+
 int arch_cpu_init(void)
 {
 	init_aips();
 
+	init_csu();
 	/* Disable PDE bit of WMCR register */
 	imx_set_wdog_powerdown(false);
 
-- 
2.5.2

[U-Boot] [PATCH 3/3] imx: config: default enable nosec mode

[Frank Li]

Enable psci and nosec linux boot.
So second core can boot at community 4.13 kernel.

Signed-off-by: Frank Li <Frank.Li@freescale.com>
---
 include/configs/mx7_common.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/configs/mx7_common.h b/include/configs/mx7_common.h
index ea2be49..3231d26 100644
--- a/include/configs/mx7_common.h
+++ b/include/configs/mx7_common.h
@@ -92,4 +92,15 @@
 #define CONFIG_CMD_FUSE
 #define CONFIG_MXC_OCOTP
 
+/* Default boot linux kernel in no secure mode.
+ * If want to boot kernel in secure mode, please define CONFIG_MX7_SEC
+ */
+#ifndef CONFIG_MX7_SEC
+#define CONFIG_ARMV7_VIRT 1
+#define CONFIG_ARMV7_NONSEC 1
+#define CONFIG_ARMV7_PSCI 1
+#define CONFIG_ARMV7_PSCI_NR_CPUS	2
+#define CONFIG_ARMV7_SECURE_BASE 0x00900000
+#endif
+
 #endif
-- 
2.5.2

[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Peng Fan]

On Tue, Oct 06, 2015 at 05:13:24PM -0500, Frank Li wrote:
>When added above configuration, iram fix up plus relocate offset may locate
>in invalidate space. Write back fix up value will cause data abort.
>
>Add address check, skip psci code.
>
>Signed-off-by: Frank Li <Frank.Li@freescale.com>
>---
> arch/arm/lib/relocate.S | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
>index 475d503..6795a1b 100644
>--- a/arch/arm/lib/relocate.S
>+++ b/arch/arm/lib/relocate.S
>@@ -99,6 +99,10 @@ fixloop:
> 	cmp	r1, #23			/* relative fixup? */
> 	bne	fixnext
> 
>+	ldr	r1, =__image_copy_start
>+	cmp	r0, r1
>+	blo	fixnext
>+

Hi Tom, Albert,

This is a bug fix, please consider to apply this patch.

The secure code such as PSCI is not relocated, so there is no need to fix the code
which generate relocate entry in rel.dyn section. We should only need take
code from __image_copy_start to __image_copy_end into consideration.

Regards,
Peng.

> 	/* relative fix: increase location by offset */
> 	add	r0, r0, r4
> 	ldr	r1, [r0]
>-- 
>2.5.2
>
>_______________________________________________
>U-Boot mailing list
>U-Boot at lists.denx.de
>http://lists.denx.de/mailman/listinfo/u-boot

-- 

[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Albert ARIBAUD]

Hello Peng,

On Mon, 19 Oct 2015 13:40:51 +0800, Peng Fan <b51431@freescale.com>
wrote:
> On Tue, Oct 06, 2015 at 05:13:24PM -0500, Frank Li wrote:
> >When added above configuration, iram fix up plus relocate offset may locate
> >in invalidate space. Write back fix up value will cause data abort.
> >
> >Add address check, skip psci code.
> >
> >Signed-off-by: Frank Li <Frank.Li@freescale.com>
> >---
> > arch/arm/lib/relocate.S | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> >diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
> >index 475d503..6795a1b 100644
> >--- a/arch/arm/lib/relocate.S
> >+++ b/arch/arm/lib/relocate.S
> >@@ -99,6 +99,10 @@ fixloop:
> > 	cmp	r1, #23			/* relative fixup? */
> > 	bne	fixnext
> > 
> >+	ldr	r1, =__image_copy_start
> >+	cmp	r0, r1
> >+	blo	fixnext
> >+
> 
> Hi Tom, Albert,
> 
> This is a bug fix, please consider to apply this patch.

Sorry for not spotting your patch earlier.

Took me some time to understand the commit summary. Let me see if I'm
getting this right by paraphrasing it:

	If CONFIG_ARMV7_SECURE_BASE is defined and if there is
	a fixup to the location at __image_copy_start, then U-Boot
	will try to fix that location and since it is write-protected,
	it will result in a data abort.

If I got this right, then this raises the following questions:

1) if this location cannot be written into for relocation fixup, then
how could it be written into for the copying which precedes relocation?

2) if there is a fixup to the location, it means that either this
location will not work properly without a fix, or the compiler emitted
an erroneous relocation record. In either case, just ignoring the fixup
seems like papering over the issue.

Besides, this patch will prevent image base fixups on all targets, even
those for which CONFIG_ARMV7_SECURE_BASE is not defined.

So, for now, I would NAK it.

Now, assuming the answers to the two questions above do make a valid
point that the fix above should indeed be implemented, then:

> The secure code such as PSCI is not relocated, so there is no need to fix the code
> which generate relocate entry in rel.dyn section. We should only need take
> code from __image_copy_start to __image_copy_end into consideration.

If some part of an image needs copying but not relocating, then the
right solution is not to hard-code some arbitrary location as 'not
relocatable' in the relocation routine; the right solution is to
put in place a generic mechanism to allow the linker script to define
which part of the image is relocatable. This can be done as follows:

- in the linker script, border the non-relocatable part of the image
  with two symbols, say 'relocatable_image_start' and '..._end', and
  ensure that text (code) which should *not* be relocated is mapped
  either before '..._start' or '..._end'. Not-to-be-relocated code
  would be recognized by its text section name, e.g. '.nonreloc.text*'.

- in the relocation routine, only fix up those locations which lie
  between the 'relocatable_image_start' and '..._end' symbols.

- in the relevant makefile(s), make the non-relocatable object files
  use a test section name of the for defined in the first step (e.g.
  '.nonreloc.text*').

Again, this is suggested *only* if it is shown that the data abort
cannot be avoided some other way.

> Regards,
> Peng.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Peng Fan]

Hi Albert,
On Mon, Oct 19, 2015 at 08:48:56AM +0200, Albert ARIBAUD wrote:
>Hello Peng,
>
>On Mon, 19 Oct 2015 13:40:51 +0800, Peng Fan <b51431@freescale.com>
>wrote:
>> On Tue, Oct 06, 2015 at 05:13:24PM -0500, Frank Li wrote:
>> >When added above configuration, iram fix up plus relocate offset may locate
>> >in invalidate space. Write back fix up value will cause data abort.
>> >
>> >Add address check, skip psci code.
>> >
>> >Signed-off-by: Frank Li <Frank.Li@freescale.com>
>> >---
>> > arch/arm/lib/relocate.S | 4 ++++
>> > 1 file changed, 4 insertions(+)
>> >
>> >diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
>> >index 475d503..6795a1b 100644
>> >--- a/arch/arm/lib/relocate.S
>> >+++ b/arch/arm/lib/relocate.S
>> >@@ -99,6 +99,10 @@ fixloop:
>> > 	cmp	r1, #23			/* relative fixup? */
>> > 	bne	fixnext
>> > 
>> >+	ldr	r1, =__image_copy_start
>> >+	cmp	r0, r1
>> >+	blo	fixnext
>> >+
>> 
>> Hi Tom, Albert,
>> 
>> This is a bug fix, please consider to apply this patch.
>
>Sorry for not spotting your patch earlier.

Not from me -:)

>
>Took me some time to understand the commit summary. Let me see if I'm
>getting this right by paraphrasing it:
>
>	If CONFIG_ARMV7_SECURE_BASE is defined and if there is
>	a fixup to the location at __image_copy_start, then U-Boot
>	will try to fix that location and since it is write-protected,
>	it will result in a data abort.

The commit msg needs to be refined.

>
>If I got this right, then this raises the following questions:
>
>1) if this location cannot be written into for relocation fixup, then
>how could it be written into for the copying which precedes relocation?
>
>2) if there is a fixup to the location, it means that either this
>location will not work properly without a fix, or the compiler emitted
>an erroneous relocation record. In either case, just ignoring the fixup
>seems like papering over the issue.
>
>Besides, this patch will prevent image base fixups on all targets, even
>those for which CONFIG_ARMV7_SECURE_BASE is not defined.

From my understanding, U-Boot will be relocated to high address of the DRAM space,
exactly code from __image_copy_start to __image_copy_end.

Following is part of the dump of .rel.dyn section of u-boot elf.
SECURE BASE is 0x180000, uboot entry is 0x87800000.

Relocation section '.rel.dyn' at offset 0x577d4 contains 4032 entries:
 Offset     Info    Type            Sym.Value  Sym. Name
 0018410c  00000017 R_ARM_RELATIVE   
 00184154  00000017 R_ARM_RELATIVE   
 0018415c  00000017 R_ARM_RELATIVE   
 00184164  00000017 R_ARM_RELATIVE   
 0018416c  00000017 R_ARM_RELATIVE   
 00184304  00000017 R_ARM_RELATIVE   
 00184368  00000017 R_ARM_RELATIVE   
 87800020  00000017 R_ARM_RELATIVE   
 87800024  00000017 R_ARM_RELATIVE   
 87800028  00000017 R_ARM_RELATIVE   
 8780002c  00000017 R_ARM_RELATIVE

We can see that there are several relocate entries for secure code which is not
copied to high address of DRAM.

However SECURE code will not be copied to high address and should not be copied.
The code locates from __image_copy_start to __image_copy_end are copied to high
address, after copied, we need to fix variable and function reference, means
we need to fix this according to the relocation entry. The relocation entry
for SECURE code is not needed and should not to be fixed, alought they are in
the rel.dyn section.

>
>So, for now, I would NAK it.
>
>Now, assuming the answers to the two questions above do make a valid
>point that the fix above should indeed be implemented, then:
>
>> The secure code such as PSCI is not relocated, so there is no need to fix the code
>> which generate relocate entry in rel.dyn section. We should only need take
>> code from __image_copy_start to __image_copy_end into consideration.
>
>If some part of an image needs copying but not relocating,then the

part of an image not needs copying and not needs relocating.

>right solution is not to hard-code some arbitrary location as 'not
>relocatable' in the relocation routine; the right solution is to
>put in place a generic mechanism to allow the linker script to define
>which part of the image is relocatable. This can be done as follows:
>
>- in the linker script, border the non-relocatable part of the image
>  with two symbols, say 'relocatable_image_start' and '..._end', and
>  ensure that text (code) which should *not* be relocated is mapped
>  either before '..._start' or '..._end'. Not-to-be-relocated code
>  would be recognized by its text section name, e.g. '.nonreloc.text*'.

We have another fix is to add the following linker in u-boot.lds for arm
+	.rel.secure :
+	{
+		*(.rel._secure*)
+	}

Then relocation entry for secure code will not be touched.

But from my point, when doing relocation fix like the following c code:

if ((entry < __image_copy_end)  && (entry > __image_copy_start)) {
     /* fixup according relocation entry */
}

Regards,
Peng.

>
>- in the relocation routine, only fix up those locations which lie
>  between the 'relocatable_image_start' and '..._end' symbols.
>
>- in the relevant makefile(s), make the non-relocatable object files
>  use a test section name of the for defined in the first step (e.g.
>  '.nonreloc.text*').
>
>Again, this is suggested *only* if it is shown that the data abort
>cannot be avoided some other way.
>
>> Regards,
>> Peng.
>
>Amicalement,
>-- 
>Albert.

-- 

[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Albert ARIBAUD]

Hello Peng,

On Mon, 19 Oct 2015 15:19:09 +0800, Peng Fan <b51431@freescale.com>
wrote:
> Hi Albert,
> On Mon, Oct 19, 2015 at 08:48:56AM +0200, Albert ARIBAUD wrote:
> >Hello Peng,
> >
> >On Mon, 19 Oct 2015 13:40:51 +0800, Peng Fan <b51431@freescale.com>
> >wrote:
> >> On Tue, Oct 06, 2015 at 05:13:24PM -0500, Frank Li wrote:
> >> >When added above configuration, iram fix up plus relocate offset may locate
> >> >in invalidate space. Write back fix up value will cause data abort.
> >> >
> >> >Add address check, skip psci code.
> >> >
> >> >Signed-off-by: Frank Li <Frank.Li@freescale.com>
> >> >---
> >> > arch/arm/lib/relocate.S | 4 ++++
> >> > 1 file changed, 4 insertions(+)
> >> >
> >> >diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
> >> >index 475d503..6795a1b 100644
> >> >--- a/arch/arm/lib/relocate.S
> >> >+++ b/arch/arm/lib/relocate.S
> >> >@@ -99,6 +99,10 @@ fixloop:
> >> > 	cmp	r1, #23			/* relative fixup? */
> >> > 	bne	fixnext
> >> > 
> >> >+	ldr	r1, =__image_copy_start
> >> >+	cmp	r0, r1
> >> >+	blo	fixnext
> >> >+
> >> 
> >> Hi Tom, Albert,
> >> 
> >> This is a bug fix, please consider to apply this patch.
> >
> >Sorry for not spotting your patch earlier.
> 
> Not from me -:)
> 
> >
> >Took me some time to understand the commit summary. Let me see if I'm
> >getting this right by paraphrasing it:
> >
> >	If CONFIG_ARMV7_SECURE_BASE is defined and if there is
> >	a fixup to the location at __image_copy_start, then U-Boot
> >	will try to fix that location and since it is write-protected,
> >	it will result in a data abort.
> 
> The commit msg needs to be refined.
> 
> >
> >If I got this right, then this raises the following questions:
> >
> >1) if this location cannot be written into for relocation fixup, then
> >how could it be written into for the copying which precedes relocation?
> >
> >2) if there is a fixup to the location, it means that either this
> >location will not work properly without a fix, or the compiler emitted
> >an erroneous relocation record. In either case, just ignoring the fixup
> >seems like papering over the issue.
> >
> >Besides, this patch will prevent image base fixups on all targets, even
> >those for which CONFIG_ARMV7_SECURE_BASE is not defined.
> 
> From my understanding, U-Boot will be relocated to high address of the DRAM space,
> exactly code from __image_copy_start to __image_copy_end.

Correct [minus the pedantic nitpick that __image_copy_start is included
in the copy while __image_copy_end is excluded from it].

> Following is part of the dump of .rel.dyn section of u-boot elf.
> SECURE BASE is 0x180000, uboot entry is 0x87800000.
> 
> Relocation section '.rel.dyn' at offset 0x577d4 contains 4032 entries:
>  Offset     Info    Type            Sym.Value  Sym. Name
>  0018410c  00000017 R_ARM_RELATIVE   
>  00184154  00000017 R_ARM_RELATIVE   
>  0018415c  00000017 R_ARM_RELATIVE   
>  00184164  00000017 R_ARM_RELATIVE   
>  0018416c  00000017 R_ARM_RELATIVE   
>  00184304  00000017 R_ARM_RELATIVE   
>  00184368  00000017 R_ARM_RELATIVE   
>  87800020  00000017 R_ARM_RELATIVE   
>  87800024  00000017 R_ARM_RELATIVE   
>  87800028  00000017 R_ARM_RELATIVE   
>  8780002c  00000017 R_ARM_RELATIVE
> 
> We can see that there are several relocate entries for secure code which is not
> copied to high address of DRAM.
> 
> However SECURE code will not be copied to high address and should not be copied.
> The code locates from __image_copy_start to __image_copy_end are copied to high
> address, after copied, we need to fix variable and function reference, means
> we need to fix this according to the relocation entry. The relocation entry
> for SECURE code is not needed and should not to be fixed, alought they are in
> the rel.dyn section.

Thanks, this makes the situation much clearer -- though not entirely.

Relocation is done by:

a) compiling and linking the image as relocatable code;

a) linking the image for loading and execution at the link-time base
   address (here 0x87800000); the image won't work at any other address
   without relocation;

b) copying the code from wherever it was loaded (which may or may not
   be the link-time base address) to its run-time base address (as high
   in DDR as possible); this makes the code unfit for execution until it
   is relocated [pedantry: ... unless the run-time base address happens
   to be the link-time base address] ;

c) fixing the image based on the relocation records, the link-time base
   address, and the run-time base address; this makes the code fit for
   execution again.

So obviously, phase c should only operate on code which was linked
between __image_copy_start [included] and __image_copy_end [excluded].

This could be an argument for checking relocation targets against those
limits... But then, only code that *requires* relocation should have
any relocation record -- IOW, there is no reason for the PSCI code to
have such records if it is not going to be relocated like the rest of
the image.

This, in turn, leads to new questions:

1. How is this PSCI code put in place? Is it bundled with the image,
   with a specificy copy routine which puts it in place then locks the
   memory range against writing? Or is it loaded in place by some other
   agent than U-Boot, and how does this agent know what to put where?

2. Does this code and the relocatable image refer to symbols of one
   another, or do they interact through an IPC independent of their
   respective location (in which case, one wonders why they are built
   into a single ELF file)?

More generally... which target do you see this problem on? Reproducing
the build myself would save both you and me some time, I guess. :)

> >So, for now, I would NAK it.
> >
> >Now, assuming the answers to the two questions above do make a valid
> >point that the fix above should indeed be implemented, then:
> >
> >> The secure code such as PSCI is not relocated, so there is no need to fix the code
> >> which generate relocate entry in rel.dyn section. We should only need take
> >> code from __image_copy_start to __image_copy_end into consideration.
> >
> >If some part of an image needs copying but not relocating,then the
> 
> part of an image not needs copying and not needs relocating.

This seems to imply the PSCI code is not even put in place by U-Boot.
So how does it end up in place?

> >right solution is not to hard-code some arbitrary location as 'not
> >relocatable' in the relocation routine; the right solution is to
> >put in place a generic mechanism to allow the linker script to define
> >which part of the image is relocatable. This can be done as follows:
> >
> >- in the linker script, border the non-relocatable part of the image
> >  with two symbols, say 'relocatable_image_start' and '..._end', and
> >  ensure that text (code) which should *not* be relocated is mapped
> >  either before '..._start' or '..._end'. Not-to-be-relocated code
> >  would be recognized by its text section name, e.g. '.nonreloc.text*'.
> 
> We have another fix is to add the following linker in u-boot.lds for arm
> +	.rel.secure :
> +	{
> +		*(.rel._secure*)
> +	}

Do you need these relocation records? If not, then just append the
'*(.rel._secure*) input section spec (with a suitable comment) to
the already existing DISCARD output section. By the way, doesn't this
linker script change alone fix the data abort?

Still, if there *are* relocation records emitted, that's because the
toolchain considered them necessary -- IOW, it was (wrongly) told the
PSCI code must be relocatable [or the code really is relocatable and we
just haven't not hit a bug about this yet].

So the real root cause fix would be to add suitable compiler and linker
flags to the PSCI object files so that they are generated as really non-
relocatable.

(still, I am wondering how this code ends up in place without U-Boot at
least copying it from the loaded image into its target location; I'll
certainly get a better understanding of it once I know which target
we're talking about.)

> Then relocation entry for secure code will not be touched.
> 
> But from my point, when doing relocation fix like the following c code:
> 
> if ((entry < __image_copy_end)  && (entry > __image_copy_start)) {
>      /* fixup according relocation entry */
> }

Testing every single relocation record target against __image_copy_start
and __image_copy_end would be rather costly in the tight loop of the
relocation routine, which should run as fast as possible.

There is at least one fix to your problem which removes the out-of-
bounds relocation records, and quite probably one better fix which
prevents generating them in the first place.

Therefore, instead of the currently proposed patch which increases the
size (very slightly) and boot time (statistically in O(n) proportion)
of the image, I prefer one of the two alternative solutions which
decrease the image size (by removing useless relocation records) and
leave the boot time unchanged (since the fix is at build time only).

> Regards,
> Peng.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Peng Fan]

Hi Albert,
On Mon, Oct 19, 2015 at 10:23:40AM +0200, Albert ARIBAUD wrote:
>Hello Peng,
>
>On Mon, 19 Oct 2015 15:19:09 +0800, Peng Fan <b51431@freescale.com>
>wrote:
>> Hi Albert,
>> On Mon, Oct 19, 2015 at 08:48:56AM +0200, Albert ARIBAUD wrote:
>> >Hello Peng,
>> >
>> >On Mon, 19 Oct 2015 13:40:51 +0800, Peng Fan <b51431@freescale.com>
>> >wrote:
>> >> On Tue, Oct 06, 2015 at 05:13:24PM -0500, Frank Li wrote:
>> >> >When added above configuration, iram fix up plus relocate offset may locate
>> >> >in invalidate space. Write back fix up value will cause data abort.
>> >> >
>> >> >Add address check, skip psci code.
>> >> >
>> >> >Signed-off-by: Frank Li <Frank.Li@freescale.com>
>> >> >---
>> >> > arch/arm/lib/relocate.S | 4 ++++
>> >> > 1 file changed, 4 insertions(+)
>> >> >
>> >> >diff --git a/arch/arm/lib/relocate.S b/arch/arm/lib/relocate.S
>> >> >index 475d503..6795a1b 100644
>> >> >--- a/arch/arm/lib/relocate.S
>> >> >+++ b/arch/arm/lib/relocate.S
>> >> >@@ -99,6 +99,10 @@ fixloop:
>> >> > 	cmp	r1, #23			/* relative fixup? */
>> >> > 	bne	fixnext
>> >> > 
>> >> >+	ldr	r1, =__image_copy_start
>> >> >+	cmp	r0, r1
>> >> >+	blo	fixnext
>> >> >+
>> >> 
>> >> Hi Tom, Albert,
>> >> 
>> >> This is a bug fix, please consider to apply this patch.
>> >
>> >Sorry for not spotting your patch earlier.
>> 
>> Not from me -:)
>> 
>> >
>> >Took me some time to understand the commit summary. Let me see if I'm
>> >getting this right by paraphrasing it:
>> >
>> >	If CONFIG_ARMV7_SECURE_BASE is defined and if there is
>> >	a fixup to the location at __image_copy_start, then U-Boot
>> >	will try to fix that location and since it is write-protected,
>> >	it will result in a data abort.
>> 
>> The commit msg needs to be refined.
>> 
>> >
>> >If I got this right, then this raises the following questions:
>> >
>> >1) if this location cannot be written into for relocation fixup, then
>> >how could it be written into for the copying which precedes relocation?
>> >
>> >2) if there is a fixup to the location, it means that either this
>> >location will not work properly without a fix, or the compiler emitted
>> >an erroneous relocation record. In either case, just ignoring the fixup
>> >seems like papering over the issue.
>> >
>> >Besides, this patch will prevent image base fixups on all targets, even
>> >those for which CONFIG_ARMV7_SECURE_BASE is not defined.
>> 
>> From my understanding, U-Boot will be relocated to high address of the DRAM space,
>> exactly code from __image_copy_start to __image_copy_end.
>
>Correct [minus the pedantic nitpick that __image_copy_start is included
>in the copy while __image_copy_end is excluded from it].
>
>> Following is part of the dump of .rel.dyn section of u-boot elf.
>> SECURE BASE is 0x180000, uboot entry is 0x87800000.
>> 
>> Relocation section '.rel.dyn' at offset 0x577d4 contains 4032 entries:
>>  Offset     Info    Type            Sym.Value  Sym. Name
>>  0018410c  00000017 R_ARM_RELATIVE   
>>  00184154  00000017 R_ARM_RELATIVE   
>>  0018415c  00000017 R_ARM_RELATIVE   
>>  00184164  00000017 R_ARM_RELATIVE   
>>  0018416c  00000017 R_ARM_RELATIVE   
>>  00184304  00000017 R_ARM_RELATIVE   
>>  00184368  00000017 R_ARM_RELATIVE   
>>  87800020  00000017 R_ARM_RELATIVE   
>>  87800024  00000017 R_ARM_RELATIVE   
>>  87800028  00000017 R_ARM_RELATIVE   
>>  8780002c  00000017 R_ARM_RELATIVE
>> 
>> We can see that there are several relocate entries for secure code which is not
>> copied to high address of DRAM.
>> 
>> However SECURE code will not be copied to high address and should not be copied.
>> The code locates from __image_copy_start to __image_copy_end are copied to high
>> address, after copied, we need to fix variable and function reference, means
>> we need to fix this according to the relocation entry. The relocation entry
>> for SECURE code is not needed and should not to be fixed, alought they are in
>> the rel.dyn section.
>
>Thanks, this makes the situation much clearer -- though not entirely.
>
>Relocation is done by:
>
>a) compiling and linking the image as relocatable code;
>
>a) linking the image for loading and execution at the link-time base
>   address (here 0x87800000); the image won't work at any other address
>   without relocation;
>
>b) copying the code from wherever it was loaded (which may or may not
>   be the link-time base address) to its run-time base address (as high
>   in DDR as possible); this makes the code unfit for execution until it
>   is relocated [pedantry: ... unless the run-time base address happens
>   to be the link-time base address] ;
>
>c) fixing the image based on the relocation records, the link-time base
>   address, and the run-time base address; this makes the code fit for
>   execution again.
>
>So obviously, phase c should only operate on code which was linked
>between __image_copy_start [included] and __image_copy_end [excluded].
>
>This could be an argument for checking relocation targets against those
>limits... But then, only code that *requires* relocation should have
>any relocation record -- IOW, there is no reason for the PSCI code to
>have such records if it is not going to be relocated like the rest of
>the image.
>
>This, in turn, leads to new questions:
>
>1. How is this PSCI code put in place? Is it bundled with the image,
>   with a specificy copy routine which puts it in place then locks the
Yeah.
>   memory range against writing? Or is it loaded in place by some other
>   agent than U-Boot, and how does this agent know what to put where?

In arch/arm/cpu/armv7/virt-v7.c, relocate_secure_section() does this job.

CONFIG_ARMV7_SECURE_BASE is the location that secure code will be copied
to. To ARMV7, PSCI code is bundled with the uboot image.

>
>2. Does this code and the relocatable image refer to symbols of one
>   another,
Yeah. There is asm function reference. You can see arch/arm/cpu/armv7/psci.S
>or do they interact through an IPC independent of their
>   respective location (in which case, one wonders why they are built
>   into a single ELF file)?

This comes a question, why PSCI framework intergated into U-Boot? I have no idea.

>
>More generally... which target do you see this problem on? Reproducing
>the build myself would save both you and me some time, I guess. :)

Build target: mx7dsabresd

Needs to apply the three patches:
https://patchwork.ozlabs.org/patch/527040/
https://patchwork.ozlabs.org/patch/527038/
https://patchwork.ozlabs.org/patch/527039/

There is a small difference from my previous log, since my previous log use
0x180000 as the secure address, but the patches use 0x900000 as the secure
address. But sure there will be relocation entry there.

>
>> >So, for now, I would NAK it.
>> >
>> >Now, assuming the answers to the two questions above do make a valid
>> >point that the fix above should indeed be implemented, then:
>> >
>> >> The secure code such as PSCI is not relocated, so there is no need to fix the code
>> >> which generate relocate entry in rel.dyn section. We should only need take
>> >> code from __image_copy_start to __image_copy_end into consideration.
>> >
>> >If some part of an image needs copying but not relocating,then the
>> 
>> part of an image not needs copying and not needs relocating.
>
>This seems to imply the PSCI code is not even put in place by U-Boot.
>So how does it end up in place?

relocate_secure_section()

>
>> >right solution is not to hard-code some arbitrary location as 'not
>> >relocatable' in the relocation routine; the right solution is to
>> >put in place a generic mechanism to allow the linker script to define
>> >which part of the image is relocatable. This can be done as follows:
>> >
>> >- in the linker script, border the non-relocatable part of the image
>> >  with two symbols, say 'relocatable_image_start' and '..._end', and
>> >  ensure that text (code) which should *not* be relocated is mapped
>> >  either before '..._start' or '..._end'. Not-to-be-relocated code
>> >  would be recognized by its text section name, e.g. '.nonreloc.text*'.
>> 
>> We have another fix is to add the following linker in u-boot.lds for arm
>> +	.rel.secure :
>> +	{
>> +		*(.rel._secure*)
>> +	}
>
>Do you need these relocation records? If not, then just append the
>'*(.rel._secure*) input section spec (with a suitable comment) to
>the already existing DISCARD output section. By the way, doesn't this
>linker script change alone fix the data abort?

Yeah. Since the relocation entry for secure section will be stored
in rel.secure section. And this section will not be touched in relocate.S

>
>Still, if there *are* relocation records emitted, that's because the
>toolchain considered them necessary -- IOW, it was (wrongly) told the
>PSCI code must be relocatable [or the code really is relocatable and we
>just haven't not hit a bug about this yet].

The PSCI code is bundled with the u-boot image. But it's running address
is not same with u-boot. In my example, u-boot is compiled with starting
address 0x87800000, but to PSCI, it's running address is 0x180000.
relocate_secure_section is just copy the psci code from uboot to 0x180000.

compliation address is same with running address, to PSCI part.

>
>So the real root cause fix would be to add suitable compiler and linker
>flags to the PSCI object files so that they are generated as really non-
>relocatable.
>
>(still, I am wondering how this code ends up in place without U-Boot at
>least copying it from the loaded image into its target location; I'll
>certainly get a better understanding of it once I know which target
>we're talking about.)
>
>> Then relocation entry for secure code will not be touched.
>> 
>> But from my point, when doing relocation fix like the following c code:
>> 
>> if ((entry < __image_copy_end)  && (entry > __image_copy_start)) {
>>      /* fixup according relocation entry */
>> }
>
>Testing every single relocation record target against __image_copy_start
>and __image_copy_end would be rather costly in the tight loop of the
>relocation routine, which should run as fast as possible.
>
>There is at least one fix to your problem which removes the out-of-
>bounds relocation records, and quite probably one better fix which
>prevents generating them in the first place.
>
>Therefore, instead of the currently proposed patch which increases the
>size (very slightly) and boot time (statistically in O(n) proportion)
>of the image, I prefer one of the two alternative solutions which
>decrease the image size (by removing useless relocation records) and
>leave the boot time unchanged (since the fix is at build time only).

Agree.

Regards,
Peng.
>
>> Regards,
>> Peng.
>
>Amicalement,
>-- 
>Albert.

-- 

[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Albert ARIBAUD]

Hello Peng,

(cutting a bit through the previous mails quoting)

> >This, in turn, leads to new questions:
> >
> >1. How is this PSCI code put in place? Is it bundled with the image,
> >   with a specificy copy routine which puts it in place then locks the
> Yeah.
> >   memory range against writing? Or is it loaded in place by some other
> >   agent than U-Boot, and how does this agent know what to put where?
> 
> In arch/arm/cpu/armv7/virt-v7.c, relocate_secure_section() does this job.
> 
> CONFIG_ARMV7_SECURE_BASE is the location that secure code will be copied
> to. To ARMV7, PSCI code is bundled with the uboot image.

Hmm, the 'relocate' part of the function name is a somewhat non-optimal
choice, since the routine just copies data without modifying it.

Looking at relocate_secure_section(), I see that it it is called long
after the U-Boot code was relocated -- actually, it is called during
bootm.

This means that for any one of the other boards around that seem to use
PSCI, either "their" PSCI code does not have any relocations (which I
doubt), or it has but they don't cause any data abort when done by the
relocate_code() routine.

So what's the difference between those and your board? My bet is that
their CONFIG_ARMV7_SECURE_BASE is not write-protected at the time the
relocate_code() routine executes (whereas on your board it is), and
will be unlocked in some way by the time relocate_secure_section()
executes.

I'm not saying that the correct solution would be to enable write
access to CONFIG_ARMV7_SECURE_BASE before running relocate_code(),
because doing that would be obviously wrong: relocate_code() would try
to fix code which is not there yet.

I'm just saying that's what actually happens, and if I am right, then
it confirms that the correct fix is to not let relocations to the
secure stay in the U-Boot ELF, or better yet, to not let them happen to
boot [pun half-intended].

> CONFIG_ARMV7_SECURE_BASE is the location that secure code will be copied
> to. To ARMV7, PSCI code is bundled with the uboot image.
> 
> >
> >2. Does this code and the relocatable image refer to symbols of one
> >   another,
> Yeah. There is asm function reference. You can see arch/arm/cpu/armv7/psci.S

Looking at this file, I suspect that actually, PSCI is called through a
software interrupt / exception vector, not through a function name, and
the only symbolic relationship is via __secure_start and __secure_end
-- and those must (and will) be correctly relocated along with the
U-Boot image.

> >or do they interact through an IPC independent of their
> >   respective location (in which case, one wonders why they are built
> >   into a single ELF file)?
> 
> This comes a question, why PSCI framework intergated into U-Boot? I have no idea.

There could have been alternative designs, indeed. Here is the design
now, so let's handle it.

> >More generally... which target do you see this problem on? Reproducing
> >the build myself would save both you and me some time, I guess. :)
> 
> Build target: mx7dsabresd
> 
> Needs to apply the three patches:
> https://patchwork.ozlabs.org/patch/527040/
> https://patchwork.ozlabs.org/patch/527038/
> https://patchwork.ozlabs.org/patch/527039/
> 
> There is a small difference from my previous log, since my previous log use
> 0x180000 as the secure address, but the patches use 0x900000 as the secure
> address. But sure there will be relocation entry there.

Thanks. I'll try and play with compiler / linker options, but from
my own experience, this can be tricky. Meanwhile, I suggest you for for
the not-too-quick-and-not-too-dirty linker script solution.

> >Do you need these relocation records? If not, then just append the
> >'*(.rel._secure*) input section spec (with a suitable comment) to
> >the already existing DISCARD output section. By the way, doesn't this
> >linker script change alone fix the data abort?
> 
> Yeah. Since the relocation entry for secure section will be stored
> in rel.secure section. And this section will not be touched in relocate.S

Ok, so that's our first clean, linker-script-based, solution confirmed.

> >Still, if there *are* relocation records emitted, that's because the
> >toolchain considered them necessary -- IOW, it was (wrongly) told the
> >PSCI code must be relocatable [or the code really is relocatable and we
> >just haven't not hit a bug about this yet].
> 
> The PSCI code is bundled with the u-boot image. But it's running address
> is not same with u-boot. In my example, u-boot is compiled with starting
> address 0x87800000, but to PSCI, it's running address is 0x180000.
> relocate_secure_section is just copy the psci code from uboot to 0x180000.
> 
> compliation address is same with running address, to PSCI part.

(I understand that. My question was not "why is PSCI built for its
run-time address?" but "Since PSCI is built for its run-time address,
and since there is little dependency on how come it is built as
relocatable?")

> >Testing every single relocation record target against __image_copy_start
> >and __image_copy_end would be rather costly in the tight loop of the
> >relocation routine, which should run as fast as possible.
> >
> >There is at least one fix to your problem which removes the out-of-
> >bounds relocation records, and quite probably one better fix which
> >prevents generating them in the first place.
> >
> >Therefore, instead of the currently proposed patch which increases the
> >size (very slightly) and boot time (statistically in O(n) proportion)
> >of the image, I prefer one of the two alternative solutions which
> >decrease the image size (by removing useless relocation records) and
> >leave the boot time unchanged (since the fix is at build time only).
> 
> Agree.

OK. So, for now, I suggest that you:

- resubmit v2 of this series with patch 1/3 reworked into a linker
  script change rather than a relocate routine change (and collect the
  secure relocation records input sections into the DISCARD output
  section rather than into a purposeless non-discarded section);

- make sure that you CC: the maintainers for the other boards which use
  PSCI and explicitly ask them to test the change on their boards and
  give their "Tested-by". It seems like the list of candidates for
  testing includes jetson-tk1, ls1021aqds, ls1021atwr and all sun[678]i
  boards, but I am by no means a PSCI specialist, so anyone feel free
  to correct me about this list.

> Regards,
> Peng.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Peng Fan]

Hi Albert,
On Mon, Oct 19, 2015 at 01:48:25PM +0200, Albert ARIBAUD wrote:
>Hello Peng,
>
>(cutting a bit through the previous mails quoting)
>
>> >This, in turn, leads to new questions:
>> >
>> >1. How is this PSCI code put in place? Is it bundled with the image,
>> >   with a specificy copy routine which puts it in place then locks the
>> Yeah.
>> >   memory range against writing? Or is it loaded in place by some other
>> >   agent than U-Boot, and how does this agent know what to put where?
>> 
>> In arch/arm/cpu/armv7/virt-v7.c, relocate_secure_section() does this job.
>> 
>> CONFIG_ARMV7_SECURE_BASE is the location that secure code will be copied
>> to. To ARMV7, PSCI code is bundled with the uboot image.
>
>Hmm, the 'relocate' part of the function name is a somewhat non-optimal
>choice, since the routine just copies data without modifying it.
>
>Looking at relocate_secure_section(), I see that it it is called long
>after the U-Boot code was relocated -- actually, it is called during
>bootm.

oh. You remind me, thanks. There maybe something wrong with my previous analysis.

I am not the one who finds the issue, just my understanding.
The data abort may be triggered by line 104 or line 106.
96 fixloop:
97         ldmia   r2!, {r0-r1}            /* (r0,r1) <- (SRC location,fixup) */
98         and     r1, r1, #0xff   
99         cmp     r1, #23                 /* relative fixup? */
100         bne     fixnext
101
102         /* relative fix: increase location by offset */
103         add     r0, r0, r4
104         ldr     r1, [r0]
105         add     r1, r1, r4
106         str     r1, [r0]

At line 104 or 106, the value of r0 may be an address that can not be accessed
which trigger data abort.

Frank, am I right?

I'll set up one board to test this.

>
>This means that for any one of the other boards around that seem to use
>PSCI, either "their" PSCI code does not have any relocations (which I
>doubt), or it has but they don't cause any data abort when done by the
>relocate_code() routine.

There maybe something wrong from me. See above.

>
>So what's the difference between those and your board? My bet is that
>their CONFIG_ARMV7_SECURE_BASE is not write-protected at the time the
>relocate_code() routine executes (whereas on your board it is), and
>will be unlocked in some way by the time relocate_secure_section()
>executes.
>
>I'm not saying that the correct solution would be to enable write
>access to CONFIG_ARMV7_SECURE_BASE before running relocate_code(),
>because doing that would be obviously wrong: relocate_code() would try
>to fix code which is not there yet.

Yeah. This is true, relocate_code may fix code which is not there.

>
>I'm just saying that's what actually happens, and if I am right, then
>it confirms that the correct fix is to not let relocations to the
>secure stay in the U-Boot ELF, or better yet, to not let them happen to
>boot [pun half-intended].

They do not happen to boot. My bad. See above.

>
>> CONFIG_ARMV7_SECURE_BASE is the location that secure code will be copied
>> to. To ARMV7, PSCI code is bundled with the uboot image.
>> 
>> >
>> >2. Does this code and the relocatable image refer to symbols of one
>> >   another,
>> Yeah. There is asm function reference. You can see arch/arm/cpu/armv7/psci.S
>
>Looking at this file, I suspect that actually, PSCI is called through a
>software interrupt / exception vector, not through a function name, and
>the only symbolic relationship is via __secure_start and __secure_end
>-- and those must (and will) be correctly relocated along with the
>U-Boot image.
>
>> >or do they interact through an IPC independent of their
>> >   respective location (in which case, one wonders why they are built
>> >   into a single ELF file)?
>> 
>> This comes a question, why PSCI framework intergated into U-Boot? I have no idea.
>
>There could have been alternative designs, indeed. Here is the design
>now, so let's handle it.
>
>> >More generally... which target do you see this problem on? Reproducing
>> >the build myself would save both you and me some time, I guess. :)
>> 
>> Build target: mx7dsabresd
>> 
>> Needs to apply the three patches:
>> https://patchwork.ozlabs.org/patch/527040/
>> https://patchwork.ozlabs.org/patch/527038/
>> https://patchwork.ozlabs.org/patch/527039/
>> 
>> There is a small difference from my previous log, since my previous log use
>> 0x180000 as the secure address, but the patches use 0x900000 as the secure
>> address. But sure there will be relocation entry there.
>
>Thanks. I'll try and play with compiler / linker options, but from
>my own experience, this can be tricky. Meanwhile, I suggest you for for
>the not-too-quick-and-not-too-dirty linker script solution.
>
>> >Do you need these relocation records? If not, then just append the
>> >'*(.rel._secure*) input section spec (with a suitable comment) to
>> >the already existing DISCARD output section. By the way, doesn't this
>> >linker script change alone fix the data abort?
>> 
>> Yeah. Since the relocation entry for secure section will be stored
>> in rel.secure section. And this section will not be touched in relocate.S
>
>Ok, so that's our first clean, linker-script-based, solution confirmed.
>
>> >Still, if there *are* relocation records emitted, that's because the
>> >toolchain considered them necessary -- IOW, it was (wrongly) told the
>> >PSCI code must be relocatable [or the code really is relocatable and we
>> >just haven't not hit a bug about this yet].
>> 
>> The PSCI code is bundled with the u-boot image. But it's running address
>> is not same with u-boot. In my example, u-boot is compiled with starting
>> address 0x87800000, but to PSCI, it's running address is 0x180000.
>> relocate_secure_section is just copy the psci code from uboot to 0x180000.
>> 
>> compliation address is same with running address, to PSCI part.
>
>(I understand that. My question was not "why is PSCI built for its
>run-time address?" but "Since PSCI is built for its run-time address,
>and since there is little dependency on how come it is built as
>relocatable?")

uboot will not effect after switch to kernel, and its image will be overriden
by kernel. But PSCI will always effect during kernel lifecyle.
Maybe built PSCI part non-relocatable is an alternative choice.

>
>> >Testing every single relocation record target against __image_copy_start
>> >and __image_copy_end would be rather costly in the tight loop of the
>> >relocation routine, which should run as fast as possible.
>> >
>> >There is at least one fix to your problem which removes the out-of-
>> >bounds relocation records, and quite probably one better fix which
>> >prevents generating them in the first place.
>> >
>> >Therefore, instead of the currently proposed patch which increases the
>> >size (very slightly) and boot time (statistically in O(n) proportion)
>> >of the image, I prefer one of the two alternative solutions which
>> >decrease the image size (by removing useless relocation records) and
>> >leave the boot time unchanged (since the fix is at build time only).
>> 
>> Agree.
>
>OK. So, for now, I suggest that you:
>
>- resubmit v2 of this series with patch 1/3 reworked into a linker
>  script change rather than a relocate routine change (and collect the
>  secure relocation records input sections into the DISCARD output
>  section rather than into a purposeless non-discarded section);
>
>- make sure that you CC: the maintainers for the other boards which use
>  PSCI and explicitly ask them to test the change on their boards and
>  give their "Tested-by". It seems like the list of candidates for
>  testing includes jetson-tk1, ls1021aqds, ls1021atwr and all sun[678]i
>  boards, but I am by no means a PSCI specialist, so anyone feel free
>  to correct me about this list.
>
>> Regards,
>> Peng.
>
>Amicalement,
>-- 
>Albert.

-- 

[U-Boot] [PATCH 1/3] ARM: relocate: fix hang when CONFIG_ARMV7_SECURE_BASE

[Li Frank]

> -----Original Message-----
> From: Peng Fan [mailto:b51431 at freescale.com]
> Sent: Monday, October 19, 2015 7:47 AM
> To: Albert ARIBAUD <albert.u.boot@aribaud.net>
> Cc: Li Frank-B20596 <Frank.Li@freescale.com>; lznuaa at gmail.com; u-
> boot at lists.denx.de; sbabic at denx.de; Estevam Fabio-R49496
> <Fabio.Estevam@freescale.com>; otavio at ossystems.com.br;
> trini at konsulko.com; hdegoede at redhat.com; Fan Peng-B51431
> <Peng.Fan@freescale.com>
> Subject: Re: [U-Boot] [PATCH 1/3] ARM: relocate: fix hang when
> CONFIG_ARMV7_SECURE_BASE
> 
> Hi Albert,
> On Mon, Oct 19, 2015 at 01:48:25PM +0200, Albert ARIBAUD wrote:
> >Hello Peng,
> >
> >(cutting a bit through the previous mails quoting)
> >
> >> >This, in turn, leads to new questions:
> >> >
> >> >1. How is this PSCI code put in place? Is it bundled with the image,
> >> >   with a specificy copy routine which puts it in place then locks
> >> >the
> >> Yeah.
> >> >   memory range against writing? Or is it loaded in place by some other
> >> >   agent than U-Boot, and how does this agent know what to put where?
> >>
> >> In arch/arm/cpu/armv7/virt-v7.c, relocate_secure_section() does this job.
> >>
> >> CONFIG_ARMV7_SECURE_BASE is the location that secure code will be
> >> copied to. To ARMV7, PSCI code is bundled with the uboot image.
> >
> >Hmm, the 'relocate' part of the function name is a somewhat non-optimal
> >choice, since the routine just copies data without modifying it.
> >
> >Looking at relocate_secure_section(), I see that it it is called long
> >after the U-Boot code was relocated -- actually, it is called during
> >bootm.
> 
> oh. You remind me, thanks. There maybe something wrong with my previous
> analysis.
> 
> I am not the one who finds the issue, just my understanding.
> The data abort may be triggered by line 104 or line 106.
> 96 fixloop:
> 97         ldmia   r2!, {r0-r1}            /* (r0,r1) <- (SRC location,fixup) */
> 98         and     r1, r1, #0xff
> 99         cmp     r1, #23                 /* relative fixup? */
> 100         bne     fixnext
> 101
> 102         /* relative fix: increase location by offset */
> 103         add     r0, r0, r4
> 104         ldr     r1, [r0]
> 105         add     r1, r1, r4
> 106         str     r1, [r0]
> 
> At line 104 or 106, the value of r0 may be an address that can not be accessed
> which trigger data abort.
> 
> Frank, am I right?

Yes. 
Secure_text + relocate_offset will be invalidate address. 
So generate data abort. 

Best regards
Frank Li

> 
> I'll set up one board to test this.
> 
> >
> >This means that for any one of the other boards around that seem to use
> >PSCI, either "their" PSCI code does not have any relocations (which I
> >doubt), or it has but they don't cause any data abort when done by the
> >relocate_code() routine.


Their secure_text + relocate_offset is possible in unused validate memory region.
If not data abort, this don't hurt anything. 

Best regards
Frank Li

> 
> There maybe something wrong from me. See above.
> 
> >
> >So what's the difference between those and your board? My bet is that
> >their CONFIG_ARMV7_SECURE_BASE is not write-protected at the time the
> >relocate_code() routine executes (whereas on your board it is), and
> >will be unlocked in some way by the time relocate_secure_section()
> >executes.
> >
> >I'm not saying that the correct solution would be to enable write
> >access to CONFIG_ARMV7_SECURE_BASE before running relocate_code(),
> >because doing that would be obviously wrong: relocate_code() would try
> >to fix code which is not there yet.
> 
> Yeah. This is true, relocate_code may fix code which is not there.
> 
> >
> >I'm just saying that's what actually happens, and if I am right, then
> >it confirms that the correct fix is to not let relocations to the
> >secure stay in the U-Boot ELF, or better yet, to not let them happen to
> >boot [pun half-intended].
> 
> They do not happen to boot. My bad. See above.
> 
> >
> >> CONFIG_ARMV7_SECURE_BASE is the location that secure code will be
> >> copied to. To ARMV7, PSCI code is bundled with the uboot image.
> >>
> >> >
> >> >2. Does this code and the relocatable image refer to symbols of one
> >> >   another,
> >> Yeah. There is asm function reference. You can see
> >> arch/arm/cpu/armv7/psci.S
> >
> >Looking at this file, I suspect that actually, PSCI is called through a
> >software interrupt / exception vector, not through a function name, and
> >the only symbolic relationship is via __secure_start and __secure_end
> >-- and those must (and will) be correctly relocated along with the
> >U-Boot image.
> >
> >> >or do they interact through an IPC independent of their
> >> >   respective location (in which case, one wonders why they are built
> >> >   into a single ELF file)?
> >>
> >> This comes a question, why PSCI framework intergated into U-Boot? I have
> no idea.
> >
> >There could have been alternative designs, indeed. Here is the design
> >now, so let's handle it.
> >
> >> >More generally... which target do you see this problem on?
> >> >Reproducing the build myself would save both you and me some time, I
> >> >guess. :)
> >>
> >> Build target: mx7dsabresd
> >>
> >> Needs to apply the three patches:
> >> https://patchwork.ozlabs.org/patch/527040/
> >> https://patchwork.ozlabs.org/patch/527038/
> >> https://patchwork.ozlabs.org/patch/527039/
> >>
> >> There is a small difference from my previous log, since my previous
> >> log use
> >> 0x180000 as the secure address, but the patches use 0x900000 as the
> >> secure address. But sure there will be relocation entry there.
> >
> >Thanks. I'll try and play with compiler / linker options, but from my
> >own experience, this can be tricky. Meanwhile, I suggest you for for
> >the not-too-quick-and-not-too-dirty linker script solution.
> >
> >> >Do you need these relocation records? If not, then just append the
> >> >'*(.rel._secure*) input section spec (with a suitable comment) to
> >> >the already existing DISCARD output section. By the way, doesn't
> >> >this linker script change alone fix the data abort?
> >>
> >> Yeah. Since the relocation entry for secure section will be stored in
> >> rel.secure section. And this section will not be touched in
> >> relocate.S
> >
> >Ok, so that's our first clean, linker-script-based, solution confirmed.
> >
> >> >Still, if there *are* relocation records emitted, that's because the
> >> >toolchain considered them necessary -- IOW, it was (wrongly) told
> >> >the PSCI code must be relocatable [or the code really is relocatable
> >> >and we just haven't not hit a bug about this yet].
> >>
> >> The PSCI code is bundled with the u-boot image. But it's running
> >> address is not same with u-boot. In my example, u-boot is compiled
> >> with starting address 0x87800000, but to PSCI, it's running address is
> 0x180000.
> >> relocate_secure_section is just copy the psci code from uboot to 0x180000.
> >>
> >> compliation address is same with running address, to PSCI part.
> >
> >(I understand that. My question was not "why is PSCI built for its
> >run-time address?" but "Since PSCI is built for its run-time address,
> >and since there is little dependency on how come it is built as
> >relocatable?")
> 
> uboot will not effect after switch to kernel, and its image will be overriden by
> kernel. But PSCI will always effect during kernel lifecyle.
> Maybe built PSCI part non-relocatable is an alternative choice.
> 
> >
> >> >Testing every single relocation record target against
> >> >__image_copy_start and __image_copy_end would be rather costly in
> >> >the tight loop of the relocation routine, which should run as fast as possible.
> >> >
> >> >There is at least one fix to your problem which removes the out-of-
> >> >bounds relocation records, and quite probably one better fix which
> >> >prevents generating them in the first place.
> >> >
> >> >Therefore, instead of the currently proposed patch which increases
> >> >the size (very slightly) and boot time (statistically in O(n)
> >> >proportion) of the image, I prefer one of the two alternative
> >> >solutions which decrease the image size (by removing useless
> >> >relocation records) and leave the boot time unchanged (since the fix is at
> build time only).
> >>
> >> Agree.
> >
> >OK. So, for now, I suggest that you:
> >
> >- resubmit v2 of this series with patch 1/3 reworked into a linker
> >  script change rather than a relocate routine change (and collect the
> >  secure relocation records input sections into the DISCARD output
> >  section rather than into a purposeless non-discarded section);
> >
> >- make sure that you CC: the maintainers for the other boards which use
> >  PSCI and explicitly ask them to test the change on their boards and
> >  give their "Tested-by". It seems like the list of candidates for
> >  testing includes jetson-tk1, ls1021aqds, ls1021atwr and all sun[678]i
> >  boards, but I am by no means a PSCI specialist, so anyone feel free
> >  to correct me about this list.
> >
> >> Regards,
> >> Peng.
> >
> >Amicalement,
> >--
> >Albert.
> 
> --