[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[Peng Fan]

The code such as PSCI in section named secure is bundled with
u-boot image, and when bootm, the code will be copied to their
runtime address same to compliation/linking address -
CONFIG_ARMV7_SECURE_BASE.

When compile the PSCI code and link it into the u-boot image,
there will be relocation entries in .rel.dyn section for PSCI.
Actually, we do not needs these relocation entries.

If still keep the relocation entries in .rel.dyn section,
r0 at line 103 and 106 in arch/arm/lib/relocate.S may be an invalid
address which may not support read/write for one SoC.
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]

So discard the relocation entries for code in secure section.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
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: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Tom Rini <trini@konsulko.com>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
Cc: Stefano Babic <sbabic@denx.de>
---

V1 thread: http://lists.denx.de/pipermail/u-boot/2015-October/229426.html
Changes V2:
 Refine commit msg.
 Discard the relocation entry section for secure text. 

 arch/arm/cpu/u-boot.lds | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 03cd9f6..55a0683 100644
--- a/arch/arm/cpu/u-boot.lds
+++ b/arch/arm/cpu/u-boot.lds
@@ -51,6 +51,8 @@ SECTIONS
 		*(.__secure_end)
 		LONG(0x1d1071c);	/* Must output something to reset LMA */
 	}
+
+	/DISCARD/ : { *(.rel._secure*) }
 #endif
 
 	. = ALIGN(4);
-- 
1.8.4

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

[Peng Fan]

1. add basic psci support for imx7 chip.
2. support cpu_on and cpu_off.
3. switch to non-secure mode when boot linux kernel.
4. set csu allow accessing all peripherial register in non-secure mode.

Signed-off-by: Frank Li <Frank.Li@freescale.com>
Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
---

Changes V2:
 Refine commit msg.

 arch/arm/cpu/armv7/mx7/Makefile   |  4 ++
 arch/arm/cpu/armv7/mx7/psci-mx7.c | 78 +++++++++++++++++++++++++++++++++++++++
 arch/arm/cpu/armv7/mx7/psci.S     | 54 +++++++++++++++++++++++++++
 arch/arm/cpu/armv7/mx7/soc.c      |  9 +++++
 4 files changed, 145 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..ec9ad87
--- /dev/null
+++ b/arch/arm/cpu/armv7/mx7/psci-mx7.c
@@ -0,0 +1,78 @@
+#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..34c6ab3
--- /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 2ed05ea..020731d 100644
--- a/arch/arm/cpu/armv7/mx7/soc.c
+++ b/arch/arm/cpu/armv7/mx7/soc.c
@@ -114,10 +114,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);
 
-- 
1.8.4

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

[Fabio Estevam]

On Tue, Oct 20, 2015 at 3:59 AM, Peng Fan <Peng.Fan@freescale.com> wrote:

> +static inline void psci_writel(u32 value, u32 reg)
> +{
> +       *(volatile u32 *)reg = value;
> +}
> +
> +static inline int psci_readl(u32 reg)
> +{
> +       return *(volatile u32*)reg;
> +}

Do you really need psci_writel() psci_readl() functions? Why not
simple call writel and readl instead?

> +/* 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);

Lots of magic values here. Please add defines for improving code readability.

Thanks

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

[Li Frank]

> -----Original Message-----
> From: Peng Fan [mailto:Peng.Fan at freescale.com]
> Sent: Tuesday, October 20, 2015 1:00 AM
> To: u-boot at lists.denx.de
> Cc: Fan Peng-B51431 <Peng.Fan@freescale.com>; Li Frank-B20596
> <Frank.Li@freescale.com>; Stefano Babic <sbabic@denx.de>; Estevam Fabio-
> R49496 <Fabio.Estevam@freescale.com>
> Subject: [PATCH V2 2/3] mx7: psci: add basic psci support
> 
> 1. add basic psci support for imx7 chip.
> 2. support cpu_on and cpu_off.
> 3. switch to non-secure mode when boot linux kernel.
> 4. set csu allow accessing all peripherial register in non-secure mode.
> 
> Signed-off-by: Frank Li <Frank.Li@freescale.com>
> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> ---
> 
> Changes V2:
>  Refine commit msg.
> 
>  arch/arm/cpu/armv7/mx7/Makefile   |  4 ++
>  arch/arm/cpu/armv7/mx7/psci-mx7.c | 78
> +++++++++++++++++++++++++++++++++++++++
>  arch/arm/cpu/armv7/mx7/psci.S     | 54 +++++++++++++++++++++++++++
>  arch/arm/cpu/armv7/mx7/soc.c      |  9 +++++
>  4 files changed, 145 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

Obj-y += psci-mx7.o psci.o
The otherwise psci_text_end will not be last one.

Best regards
Frank Li

> +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..ec9ad87
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/mx7/psci-mx7.c
> @@ -0,0 +1,78 @@
> +#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..34c6ab3
> --- /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 2ed05ea..020731d 100644
> --- a/arch/arm/cpu/armv7/mx7/soc.c
> +++ b/arch/arm/cpu/armv7/mx7/soc.c
> @@ -114,10 +114,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);
> 
> --
> 1.8.4
> 

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

[Albert ARIBAUD]

Hello Li,

On Tue, 20 Oct 2015 14:05:45 +0000, Li Frank <Frank.Li@freescale.com>
wrote:
> 
> 
> > -----Original Message-----
> > From: Peng Fan [mailto:Peng.Fan at freescale.com]
> > Sent: Tuesday, October 20, 2015 1:00 AM
> > To: u-boot at lists.denx.de
> > Cc: Fan Peng-B51431 <Peng.Fan@freescale.com>; Li Frank-B20596
> > <Frank.Li@freescale.com>; Stefano Babic <sbabic@denx.de>; Estevam Fabio-
> > R49496 <Fabio.Estevam@freescale.com>
> > Subject: [PATCH V2 2/3] mx7: psci: add basic psci support
> > 
> > 1. add basic psci support for imx7 chip.
> > 2. support cpu_on and cpu_off.
> > 3. switch to non-secure mode when boot linux kernel.
> > 4. set csu allow accessing all peripherial register in non-secure mode.
> > 
> > Signed-off-by: Frank Li <Frank.Li@freescale.com>
> > Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > ---
> > 
> > Changes V2:
> >  Refine commit msg.
> > 
> >  arch/arm/cpu/armv7/mx7/Makefile   |  4 ++
> >  arch/arm/cpu/armv7/mx7/psci-mx7.c | 78
> > +++++++++++++++++++++++++++++++++++++++
> >  arch/arm/cpu/armv7/mx7/psci.S     | 54 +++++++++++++++++++++++++++
> >  arch/arm/cpu/armv7/mx7/soc.c      |  9 +++++
> >  4 files changed, 145 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
> 
> Obj-y += psci-mx7.o psci.o
> The otherwise psci_text_end will not be last one.

I don't like this object module order sensitivity.

The object module order of secure code modules should not affect the
resulting binary to the point of possibly preventing it from working --
after all, the object module order of 'vanilla' image code does not
matter (1). We don't have this kind of problem when defining the image
start and end, why would we have it with the secure code start and end?

IOW, psci_text_end could (and should) be defined in the linker script,
not in an object module.

(1) except for start.S, which *must* be linked first, and even that
is not done through object order but through linker script section
order.

> Best regards
> Frank Li

Amicalement,
-- 
Albert.

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

[Li Frank]

> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> Sent: Tuesday, October 20, 2015 9:25 AM
> To: Li Frank-B20596 <Frank.Li@freescale.com>
> Cc: Fan Peng-B51431 <Peng.Fan@freescale.com>; u-boot at lists.denx.de;
> Estevam Fabio-R49496 <Fabio.Estevam@freescale.com>
> Subject: Re: [U-Boot] [PATCH V2 2/3] mx7: psci: add basic psci support
> 
> Hello Li,
> 
> On Tue, 20 Oct 2015 14:05:45 +0000, Li Frank <Frank.Li@freescale.com>
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Peng Fan [mailto:Peng.Fan at freescale.com]
> > > Sent: Tuesday, October 20, 2015 1:00 AM
> > > To: u-boot at lists.denx.de
> > > Cc: Fan Peng-B51431 <Peng.Fan@freescale.com>; Li Frank-B20596
> > > <Frank.Li@freescale.com>; Stefano Babic <sbabic@denx.de>; Estevam
> > > Fabio-
> > > R49496 <Fabio.Estevam@freescale.com>
> > > Subject: [PATCH V2 2/3] mx7: psci: add basic psci support
> > >
> > > 1. add basic psci support for imx7 chip.
> > > 2. support cpu_on and cpu_off.
> > > 3. switch to non-secure mode when boot linux kernel.
> > > 4. set csu allow accessing all peripherial register in non-secure mode.
> > >
> > > Signed-off-by: Frank Li <Frank.Li@freescale.com>
> > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> > > Cc: Stefano Babic <sbabic@denx.de>
> > > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > > ---
> > >
> > > Changes V2:
> > >  Refine commit msg.
> > >
> > >  arch/arm/cpu/armv7/mx7/Makefile   |  4 ++
> > >  arch/arm/cpu/armv7/mx7/psci-mx7.c | 78
> > > +++++++++++++++++++++++++++++++++++++++
> > >  arch/arm/cpu/armv7/mx7/psci.S     | 54 +++++++++++++++++++++++++++
> > >  arch/arm/cpu/armv7/mx7/soc.c      |  9 +++++
> > >  4 files changed, 145 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
> >
> > Obj-y += psci-mx7.o psci.o
> > The otherwise psci_text_end will not be last one.
> 
> I don't like this object module order sensitivity.
> 
> The object module order of secure code modules should not affect the resulting
> binary to the point of possibly preventing it from working -- after all, the object
> module order of 'vanilla' image code does not matter (1). We don't have this
> kind of problem when defining the image start and end, why would we have it
> with the secure code start and end?

Secure code use psci_text_end to calculate stack top point. 

> 
> IOW, psci_text_end could (and should) be defined in the linker script, not in an
> object module.

I agree.  The other platform put psci_text_end to object module. 

Best regards
Frank Li

> 
> (1) except for start.S, which *must* be linked first, and even that is not done
> through object order but through linker script section order.
> 
> > Best regards
> > Frank Li
> 
> Amicalement,
> --
> Albert.

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

[Albert ARIBAUD]

Hello Li,

(sorry for my dropping other receipients from the discussion; restoring
them

On Tue, 20 Oct 2015 15:04:40 +0000, Li Frank <Frank.Li@freescale.com>
wrote:
> 
> 
> > -----Original Message-----
> > From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> > Sent: Tuesday, October 20, 2015 9:55 AM
> > To: Li Frank-B20596 <Frank.Li@freescale.com>
> > Subject: Re: [U-Boot] [PATCH V2 2/3] mx7: psci: add basic psci support
> > 
> > Hello Li,
> > 
> > On Tue, 20 Oct 2015 14:29:46 +0000, Li Frank <Frank.Li@freescale.com>
> > wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> > > > Sent: Tuesday, October 20, 2015 9:25 AM
> > > > To: Li Frank-B20596 <Frank.Li@freescale.com>
> > > > Cc: Fan Peng-B51431 <Peng.Fan@freescale.com>; u-boot at lists.denx.de;
> > > > Estevam Fabio-R49496 <Fabio.Estevam@freescale.com>
> > > > Subject: Re: [U-Boot] [PATCH V2 2/3] mx7: psci: add basic psci
> > > > support
> > > >
> > > > Hello Li,
> > > >
> > > > On Tue, 20 Oct 2015 14:05:45 +0000, Li Frank
> > > > <Frank.Li@freescale.com>
> > > > wrote:
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Peng Fan [mailto:Peng.Fan at freescale.com]
> > > > > > Sent: Tuesday, October 20, 2015 1:00 AM
> > > > > > To: u-boot at lists.denx.de
> > > > > > Cc: Fan Peng-B51431 <Peng.Fan@freescale.com>; Li Frank-B20596
> > > > > > <Frank.Li@freescale.com>; Stefano Babic <sbabic@denx.de>;
> > > > > > Estevam
> > > > > > Fabio-
> > > > > > R49496 <Fabio.Estevam@freescale.com>
> > > > > > Subject: [PATCH V2 2/3] mx7: psci: add basic psci support
> > > > > >
> > > > > > 1. add basic psci support for imx7 chip.
> > > > > > 2. support cpu_on and cpu_off.
> > > > > > 3. switch to non-secure mode when boot linux kernel.
> > > > > > 4. set csu allow accessing all peripherial register in non-secure mode.
> > > > > >
> > > > > > Signed-off-by: Frank Li <Frank.Li@freescale.com>
> > > > > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> > > > > > Cc: Stefano Babic <sbabic@denx.de>
> > > > > > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > > > > > ---
> > > > > >
> > > > > > Changes V2:
> > > > > >  Refine commit msg.
> > > > > >
> > > > > >  arch/arm/cpu/armv7/mx7/Makefile   |  4 ++
> > > > > >  arch/arm/cpu/armv7/mx7/psci-mx7.c | 78
> > > > > > +++++++++++++++++++++++++++++++++++++++
> > > > > >  arch/arm/cpu/armv7/mx7/psci.S     | 54
> > +++++++++++++++++++++++++++
> > > > > >  arch/arm/cpu/armv7/mx7/soc.c      |  9 +++++
> > > > > >  4 files changed, 145 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
> > > > >
> > > > > Obj-y += psci-mx7.o psci.o
> > > > > The otherwise psci_text_end will not be last one.
> > > >
> > > > I don't like this object module order sensitivity.
> > > >
> > > > The object module order of secure code modules should not affect the
> > > > resulting binary to the point of possibly preventing it from working
> > > > -- after all, the object module order of 'vanilla' image code does
> > > > not matter (1). We don't have this kind of problem when defining the
> > > > image start and end, why would we have it with the secure code start and
> > end?
> > >
> > > Secure code use psci_text_end to calculate stack top point.
> > 
> > Yes, and that does not require defining psci_text_end in an object module, nor
> > does it require specific object module ordering.
> 
> If psci_text_end in mid of secure text, 
> And psci_text_end + 1k < total secure text size,   stack will overwrite the code. 

This is a good reason to allocate the PSCCI stack and create a symbol
for its top in the linker script.

> > > > IOW, psci_text_end could (and should) be defined in the linker
> > > > script, not in an object module.
> > >
> > > I agree.  The other platform put psci_text_end to object module.
> > 
> > Which 'other platform' is this?
> 
> arch/arm/cpu/armv7/sunxi
> arch/arm/cpu/armv7/ls102xa

Ok, but they all use the same routine for moving the PSCI code and
setting its stack, right?

Amicalement,
-- 
Albert.

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

[Li Frank]

> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> Sent: Tuesday, October 20, 2015 4:04 PM
> To: Li Frank-B20596 <Frank.Li@freescale.com>
> Cc: Fan Peng-B51431 <Peng.Fan@freescale.com>; Estevam Fabio-R49496
> <Fabio.Estevam@freescale.com>; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH V2 2/3] mx7: psci: add basic psci support
> 
> Hello Li,
> 
> (sorry for my dropping other receipients from the discussion; restoring them
> 
> On Tue, 20 Oct 2015 15:04:40 +0000, Li Frank <Frank.Li@freescale.com>
> wrote:
> >
> >
> > > -----Original Message-----
> > > From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> > > Sent: Tuesday, October 20, 2015 9:55 AM
> > > To: Li Frank-B20596 <Frank.Li@freescale.com>
> > > Subject: Re: [U-Boot] [PATCH V2 2/3] mx7: psci: add basic psci
> > > support
> > >
> > > Hello Li,
> > >
> > > On Tue, 20 Oct 2015 14:29:46 +0000, Li Frank
> > > <Frank.Li@freescale.com>
> > > wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Albert ARIBAUD [mailto:albert.u.boot at aribaud.net]
> > > > > Sent: Tuesday, October 20, 2015 9:25 AM
> > > > > To: Li Frank-B20596 <Frank.Li@freescale.com>
> > > > > Cc: Fan Peng-B51431 <Peng.Fan@freescale.com>;
> > > > > u-boot at lists.denx.de; Estevam Fabio-R49496
> > > > > <Fabio.Estevam@freescale.com>
> > > > > Subject: Re: [U-Boot] [PATCH V2 2/3] mx7: psci: add basic psci
> > > > > support
> > > > >
> > > > > Hello Li,
> > > > >
> > > > > On Tue, 20 Oct 2015 14:05:45 +0000, Li Frank
> > > > > <Frank.Li@freescale.com>
> > > > > wrote:
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Peng Fan [mailto:Peng.Fan at freescale.com]
> > > > > > > Sent: Tuesday, October 20, 2015 1:00 AM
> > > > > > > To: u-boot at lists.denx.de
> > > > > > > Cc: Fan Peng-B51431 <Peng.Fan@freescale.com>; Li
> > > > > > > Frank-B20596 <Frank.Li@freescale.com>; Stefano Babic
> > > > > > > <sbabic@denx.de>; Estevam
> > > > > > > Fabio-
> > > > > > > R49496 <Fabio.Estevam@freescale.com>
> > > > > > > Subject: [PATCH V2 2/3] mx7: psci: add basic psci support
> > > > > > >
> > > > > > > 1. add basic psci support for imx7 chip.
> > > > > > > 2. support cpu_on and cpu_off.
> > > > > > > 3. switch to non-secure mode when boot linux kernel.
> > > > > > > 4. set csu allow accessing all peripherial register in non-secure
> mode.
> > > > > > >
> > > > > > > Signed-off-by: Frank Li <Frank.Li@freescale.com>
> > > > > > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> > > > > > > Cc: Stefano Babic <sbabic@denx.de>
> > > > > > > Cc: Fabio Estevam <fabio.estevam@freescale.com>
> > > > > > > ---
> > > > > > >
> > > > > > > Changes V2:
> > > > > > >  Refine commit msg.
> > > > > > >
> > > > > > >  arch/arm/cpu/armv7/mx7/Makefile   |  4 ++
> > > > > > >  arch/arm/cpu/armv7/mx7/psci-mx7.c | 78
> > > > > > > +++++++++++++++++++++++++++++++++++++++
> > > > > > >  arch/arm/cpu/armv7/mx7/psci.S     | 54
> > > +++++++++++++++++++++++++++
> > > > > > >  arch/arm/cpu/armv7/mx7/soc.c      |  9 +++++
> > > > > > >  4 files changed, 145 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
> > > > > >
> > > > > > Obj-y += psci-mx7.o psci.o
> > > > > > The otherwise psci_text_end will not be last one.
> > > > >
> > > > > I don't like this object module order sensitivity.
> > > > >
> > > > > The object module order of secure code modules should not affect
> > > > > the resulting binary to the point of possibly preventing it from
> > > > > working
> > > > > -- after all, the object module order of 'vanilla' image code
> > > > > does not matter (1). We don't have this kind of problem when
> > > > > defining the image start and end, why would we have it with the
> > > > > secure code start and
> > > end?
> > > >
> > > > Secure code use psci_text_end to calculate stack top point.
> > >
> > > Yes, and that does not require defining psci_text_end in an object
> > > module, nor does it require specific object module ordering.
> >
> > If psci_text_end in mid of secure text,
> > And psci_text_end + 1k < total secure text size,   stack will overwrite the code.
> 
> This is a good reason to allocate the PSCCI stack and create a symbol for its top
> in the linker script.
> 
> > > > > IOW, psci_text_end could (and should) be defined in the linker
> > > > > script, not in an object module.
> > > >
> > > > I agree.  The other platform put psci_text_end to object module.
> > >
> > > Which 'other platform' is this?
> >
> > arch/arm/cpu/armv7/sunxi
> > arch/arm/cpu/armv7/ls102xa
> 
> Ok, but they all use the same routine for moving the PSCI code and setting its
> stack, right?

Yes.

Best regards
Frank Li
> 
> Amicalement,
> --
> Albert.

[U-Boot] [PATCH V2 3/3] imx: mx7: default enable non-secure mode

[Peng Fan]

Support PSCI and switch to non-secure mode when booting linux.

Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
Signed-off-by: Frank Li <Frank.Li@freescale.com>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Fabio Estevam <fabio.estevam@freescale.com>
---

Changes V2:
 default no enable CONFIG_ARMV7_VIRT

 include/configs/mx7_common.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/configs/mx7_common.h b/include/configs/mx7_common.h
index ea2be49..fac76bd 100644
--- a/include/configs/mx7_common.h
+++ b/include/configs/mx7_common.h
@@ -92,4 +92,14 @@
 #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_NONSEC 1
+#define CONFIG_ARMV7_PSCI 1
+#define CONFIG_ARMV7_PSCI_NR_CPUS	2
+#define CONFIG_ARMV7_SECURE_BASE 0x00900000
+#endif
+
 #endif
-- 
1.8.4

[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[Albert ARIBAUD]

Hello Peng,

On Tue, 20 Oct 2015 13:59:53 +0800, Peng Fan <Peng.Fan@freescale.com>
wrote:
> The code such as PSCI in section named secure is bundled with
> u-boot image, and when bootm, the code will be copied to their
> runtime address same to compliation/linking address -
> CONFIG_ARMV7_SECURE_BASE.
> 
> When compile the PSCI code and link it into the u-boot image,
> there will be relocation entries in .rel.dyn section for PSCI.
> Actually, we do not needs these relocation entries.
> 
> If still keep the relocation entries in .rel.dyn section,
> r0 at line 103 and 106 in arch/arm/lib/relocate.S may be an invalid
> address which may not support read/write for one SoC.
> 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]
> 
> So discard the relocation entries for code in secure section.

Actually, I'll need you to do a slight change -- that's my fault, and
karma even ensured that my own self of two years ago would be the one
to come and kick me. See:

http://lists.denx.de/pipermail/u-boot/2013-December/168652.html

Which basically is about never discarding any section in the ELF file,
and only copying a subset of the ELF sections into the binary file.

So rather than discarding the secure relocation records, let's move
them to another section as you had proposed, and thus change the line

> +	/DISCARD/ : { *(.rel._secure*) }

Into a

	.rel._secure { *(.rel._secure*) }

Placed right after the already present

	.dynamic : { *(.dynamic*) }

With my apologies for the very late realization.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[Peng Fan]

Hi Albert,

On Tue, Oct 20, 2015 at 09:05:32AM +0200, Albert ARIBAUD wrote:
>Hello Peng,
>
>On Tue, 20 Oct 2015 13:59:53 +0800, Peng Fan <Peng.Fan@freescale.com>
>wrote:
>> The code such as PSCI in section named secure is bundled with
>> u-boot image, and when bootm, the code will be copied to their
>> runtime address same to compliation/linking address -
>> CONFIG_ARMV7_SECURE_BASE.
>> 
>> When compile the PSCI code and link it into the u-boot image,
>> there will be relocation entries in .rel.dyn section for PSCI.
>> Actually, we do not needs these relocation entries.
>> 
>> If still keep the relocation entries in .rel.dyn section,
>> r0 at line 103 and 106 in arch/arm/lib/relocate.S may be an invalid
>> address which may not support read/write for one SoC.
>> 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]
>> 
>> So discard the relocation entries for code in secure section.
>
>Actually, I'll need you to do a slight change -- that's my fault, and
>karma even ensured that my own self of two years ago would be the one
>to come and kick me. See:
>
>http://lists.denx.de/pipermail/u-boot/2013-December/168652.html

Ok. Then arch/arm/cpu/armv8/u-boot.lds should also have such fix,
since lots sections are discarded in u-boot.lds for armv8.

>
>Which basically is about never discarding any section in the ELF file,
>and only copying a subset of the ELF sections into the binary file.
>
>So rather than discarding the secure relocation records, let's move
>them to another section as you had proposed, and thus change the line
>
>> +	/DISCARD/ : { *(.rel._secure*) }
>
>Into a
>
>	.rel._secure { *(.rel._secure*) }
>
>Placed right after the already present
>
>	.dynamic : { *(.dynamic*) }

It can not be placed after .dynamic, since the following is at front.
87         .rel.dyn : {
88                 *(.rel*)
89         }
So relocation entry from secure text will first placed into .rel.dyn section.

If not DISCARD, then I prefer to put ".rel.secure : { *(.rel._secure*) }"
at line 55 which is wrapped by CONFIG_ARMV7_NONSEC in arch/arm/cpu/u-boot.lds.

See following patch:

[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[Albert ARIBAUD]

Hello Peng,

On Tue, 20 Oct 2015 15:20:43 +0800, Peng Fan <b51431@freescale.com>
wrote:
> Hi Albert,
> 
> On Tue, Oct 20, 2015 at 09:05:32AM +0200, Albert ARIBAUD wrote:
> >Hello Peng,
> >
> >On Tue, 20 Oct 2015 13:59:53 +0800, Peng Fan <Peng.Fan@freescale.com>
> >wrote:
> >> The code such as PSCI in section named secure is bundled with
> >> u-boot image, and when bootm, the code will be copied to their
> >> runtime address same to compliation/linking address -
> >> CONFIG_ARMV7_SECURE_BASE.
> >> 
> >> When compile the PSCI code and link it into the u-boot image,
> >> there will be relocation entries in .rel.dyn section for PSCI.
> >> Actually, we do not needs these relocation entries.
> >> 
> >> If still keep the relocation entries in .rel.dyn section,
> >> r0 at line 103 and 106 in arch/arm/lib/relocate.S may be an invalid
> >> address which may not support read/write for one SoC.
> >> 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]
> >> 
> >> So discard the relocation entries for code in secure section.
> >
> >Actually, I'll need you to do a slight change -- that's my fault, and
> >karma even ensured that my own self of two years ago would be the one
> >to come and kick me. See:
> >
> >http://lists.denx.de/pipermail/u-boot/2013-December/168652.html
> 
> Ok. Then arch/arm/cpu/armv8/u-boot.lds should also have such fix,
> since lots sections are discarded in u-boot.lds for armv8.

You are correct, armv8 needs to be fixed, as well as zynq (and x86 but
that's outside of my province). Anyway, that's a different problem
which does not need to be addressed in this series.

> >Which basically is about never discarding any section in the ELF file,
> >and only copying a subset of the ELF sections into the binary file.
> >
> >So rather than discarding the secure relocation records, let's move
> >them to another section as you had proposed, and thus change the line
> >
> >> +	/DISCARD/ : { *(.rel._secure*) }
> >
> >Into a
> >
> >	.rel._secure { *(.rel._secure*) }
> >
> >Placed right after the already present
> >
> >	.dynamic : { *(.dynamic*) }
> 
> It can not be placed after .dynamic, since the following is at front.
> 87         .rel.dyn : {
> 88                 *(.rel*)
> 89         }
> So relocation entry from secure text will first placed into .rel.dyn section.
> 
> If not DISCARD, then I prefer to put ".rel.secure : { *(.rel._secure*) }"
> at line 55 which is wrapped by CONFIG_ARMV7_NONSEC in arch/arm/cpu/u-boot.lds.

I prefer all 'unused' sections to be kept together near the end of the
LDS file -- I'll add an explicit comment in the LDS about it.

Besides, there no need to wrap such a section with a preprocessor
conditional, as the linker will simply not emit an output section if
there are no input sections at all for it; IOW, adding the '.rel._secure
{ *(.rel._secure*) }' line will be binary-invariant for platforms which
do not have PSCI or other secure code.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[Peng Fan]

Hi Albert,

On Tue, Oct 20, 2015 at 09:32:51AM +0200, Albert ARIBAUD wrote:
>Hello Peng,
>
>On Tue, 20 Oct 2015 15:20:43 +0800, Peng Fan <b51431@freescale.com>
>wrote:
>> Hi Albert,
>> 
>> On Tue, Oct 20, 2015 at 09:05:32AM +0200, Albert ARIBAUD wrote:
>> >Hello Peng,
>> >
>> >On Tue, 20 Oct 2015 13:59:53 +0800, Peng Fan <Peng.Fan@freescale.com>
>> >wrote:
>> >> The code such as PSCI in section named secure is bundled with
>> >> u-boot image, and when bootm, the code will be copied to their
>> >> runtime address same to compliation/linking address -
>> >> CONFIG_ARMV7_SECURE_BASE.
>> >> 
>> >> When compile the PSCI code and link it into the u-boot image,
>> >> there will be relocation entries in .rel.dyn section for PSCI.
>> >> Actually, we do not needs these relocation entries.
>> >> 
>> >> If still keep the relocation entries in .rel.dyn section,
>> >> r0 at line 103 and 106 in arch/arm/lib/relocate.S may be an invalid
>> >> address which may not support read/write for one SoC.
>> >> 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]
>> >> 
>> >> So discard the relocation entries for code in secure section.
>> >
>> >Actually, I'll need you to do a slight change -- that's my fault, and
>> >karma even ensured that my own self of two years ago would be the one
>> >to come and kick me. See:
>> >
>> >http://lists.denx.de/pipermail/u-boot/2013-December/168652.html
>> 
>> Ok. Then arch/arm/cpu/armv8/u-boot.lds should also have such fix,
>> since lots sections are discarded in u-boot.lds for armv8.
>
>You are correct, armv8 needs to be fixed, as well as zynq (and x86 but
>that's outside of my province). Anyway, that's a different problem
>which does not need to be addressed in this series.
>
>> >Which basically is about never discarding any section in the ELF file,
>> >and only copying a subset of the ELF sections into the binary file.
>> >
>> >So rather than discarding the secure relocation records, let's move
>> >them to another section as you had proposed, and thus change the line
>> >
>> >> +	/DISCARD/ : { *(.rel._secure*) }
>> >
>> >Into a
>> >
>> >	.rel._secure { *(.rel._secure*) }
>> >
>> >Placed right after the already present
>> >
>> >	.dynamic : { *(.dynamic*) }
>> 
>> It can not be placed after .dynamic, since the following is at front.
>> 87         .rel.dyn : {
>> 88                 *(.rel*)
>> 89         }
>> So relocation entry from secure text will first placed into .rel.dyn section.
>> 
>> If not DISCARD, then I prefer to put ".rel.secure : { *(.rel._secure*) }"
>> at line 55 which is wrapped by CONFIG_ARMV7_NONSEC in arch/arm/cpu/u-boot.lds.
>
>I prefer all 'unused' sections to be kept together near the end of the
>LDS file -- I'll add an explicit comment in the LDS about it.
>
>Besides, there no need to wrap such a section with a preprocessor
>conditional, as the linker will simply not emit an output section if
>there are no input sections at all for it; IOW, adding the '.rel._secure
>{ *(.rel._secure*) }' line will be binary-invariant for platforms which
>do not have PSCI or other secure code.

But ".rel._secure { *(.rel._secure*) }" can not be placed after
".dynamic : { *(.dynamic*) }". Actually ".rel._secure { *(.rel._secure*) }"
should be placed before ".rel_dyn_start" in u-boot.lds, otherwise
the secure relocation entries will be placed into ".rel.dyn", but not
".rel._secure".

Regards,
Peng.

>
>Amicalement,
>-- 
>Albert.

-- 

[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[Albert ARIBAUD]

Hello Peng,

On Tue, 20 Oct 2015 15:41:30 +0800, Peng Fan <b51431@freescale.com>
wrote:
> Hi Albert,
> 
> On Tue, Oct 20, 2015 at 09:32:51AM +0200, Albert ARIBAUD wrote:
> >Hello Peng,
> >
> >On Tue, 20 Oct 2015 15:20:43 +0800, Peng Fan <b51431@freescale.com>
> >wrote:
> >> Hi Albert,
> >> 
> >> On Tue, Oct 20, 2015 at 09:05:32AM +0200, Albert ARIBAUD wrote:
> >> >Hello Peng,
> >> >
> >> >On Tue, 20 Oct 2015 13:59:53 +0800, Peng Fan <Peng.Fan@freescale.com>
> >> >wrote:
> >> >> The code such as PSCI in section named secure is bundled with
> >> >> u-boot image, and when bootm, the code will be copied to their
> >> >> runtime address same to compliation/linking address -
> >> >> CONFIG_ARMV7_SECURE_BASE.
> >> >> 
> >> >> When compile the PSCI code and link it into the u-boot image,
> >> >> there will be relocation entries in .rel.dyn section for PSCI.
> >> >> Actually, we do not needs these relocation entries.
> >> >> 
> >> >> If still keep the relocation entries in .rel.dyn section,
> >> >> r0 at line 103 and 106 in arch/arm/lib/relocate.S may be an invalid
> >> >> address which may not support read/write for one SoC.
> >> >> 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]
> >> >> 
> >> >> So discard the relocation entries for code in secure section.
> >> >
> >> >Actually, I'll need you to do a slight change -- that's my fault, and
> >> >karma even ensured that my own self of two years ago would be the one
> >> >to come and kick me. See:
> >> >
> >> >http://lists.denx.de/pipermail/u-boot/2013-December/168652.html
> >> 
> >> Ok. Then arch/arm/cpu/armv8/u-boot.lds should also have such fix,
> >> since lots sections are discarded in u-boot.lds for armv8.
> >
> >You are correct, armv8 needs to be fixed, as well as zynq (and x86 but
> >that's outside of my province). Anyway, that's a different problem
> >which does not need to be addressed in this series.
> >
> >> >Which basically is about never discarding any section in the ELF file,
> >> >and only copying a subset of the ELF sections into the binary file.
> >> >
> >> >So rather than discarding the secure relocation records, let's move
> >> >them to another section as you had proposed, and thus change the line
> >> >
> >> >> +	/DISCARD/ : { *(.rel._secure*) }
> >> >
> >> >Into a
> >> >
> >> >	.rel._secure { *(.rel._secure*) }
> >> >
> >> >Placed right after the already present
> >> >
> >> >	.dynamic : { *(.dynamic*) }
> >> 
> >> It can not be placed after .dynamic, since the following is at front.
> >> 87         .rel.dyn : {
> >> 88                 *(.rel*)
> >> 89         }
> >> So relocation entry from secure text will first placed into .rel.dyn section.
> >> 
> >> If not DISCARD, then I prefer to put ".rel.secure : { *(.rel._secure*) }"
> >> at line 55 which is wrapped by CONFIG_ARMV7_NONSEC in arch/arm/cpu/u-boot.lds.
> >
> >I prefer all 'unused' sections to be kept together near the end of the
> >LDS file -- I'll add an explicit comment in the LDS about it.
> >
> >Besides, there no need to wrap such a section with a preprocessor
> >conditional, as the linker will simply not emit an output section if
> >there are no input sections at all for it; IOW, adding the '.rel._secure
> >{ *(.rel._secure*) }' line will be binary-invariant for platforms which
> >do not have PSCI or other secure code.
> 
> But ".rel._secure { *(.rel._secure*) }" can not be placed after
> ".dynamic : { *(.dynamic*) }". Actually ".rel._secure { *(.rel._secure*) }"
> should be placed before ".rel_dyn_start" in u-boot.lds, otherwise
> the secure relocation entries will be placed into ".rel.dyn", but not
> ".rel._secure".

Hmm, you're correct, the linker will use the first pattern match, not
the longest or best. :(

But then, the secure reloc input section cannot go in line 55, because
that's still inside the binary part of the image, which means it will
waste space in there.

So it's either use DISCARD at the very start of the SECTIONS (round
line 17) or back to not generating relocatable code at all for PSCI.

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



Amicalement,
-- 
Albert.

[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[Peng Fan]

Hello Albert,

On Tue, Oct 20, 2015 at 02:59:10PM +0200, Albert ARIBAUD wrote:
>Hello Peng,
>
>On Tue, 20 Oct 2015 15:41:30 +0800, Peng Fan <b51431@freescale.com>
>wrote:
>> Hi Albert,
>> 
>> On Tue, Oct 20, 2015 at 09:32:51AM +0200, Albert ARIBAUD wrote:
>> >Hello Peng,
>> >
>> >On Tue, 20 Oct 2015 15:20:43 +0800, Peng Fan <b51431@freescale.com>
>> >wrote:
>> >> Hi Albert,
>> >> 
>> >> On Tue, Oct 20, 2015 at 09:05:32AM +0200, Albert ARIBAUD wrote:
>> >> >Hello Peng,
>> >> >
>> >> >On Tue, 20 Oct 2015 13:59:53 +0800, Peng Fan <Peng.Fan@freescale.com>
>> >> >wrote:
>> >> >> The code such as PSCI in section named secure is bundled with
>> >> >> u-boot image, and when bootm, the code will be copied to their
>> >> >> runtime address same to compliation/linking address -
>> >> >> CONFIG_ARMV7_SECURE_BASE.
>> >> >> 
>> >> >> When compile the PSCI code and link it into the u-boot image,
>> >> >> there will be relocation entries in .rel.dyn section for PSCI.
>> >> >> Actually, we do not needs these relocation entries.
>> >> >> 
>> >> >> If still keep the relocation entries in .rel.dyn section,
>> >> >> r0 at line 103 and 106 in arch/arm/lib/relocate.S may be an invalid
>> >> >> address which may not support read/write for one SoC.
>> >> >> 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]
>> >> >> 
>> >> >> So discard the relocation entries for code in secure section.
>> >> >
>> >> >Actually, I'll need you to do a slight change -- that's my fault, and
>> >> >karma even ensured that my own self of two years ago would be the one
>> >> >to come and kick me. See:
>> >> >
>> >> >http://lists.denx.de/pipermail/u-boot/2013-December/168652.html
>> >> 
>> >> Ok. Then arch/arm/cpu/armv8/u-boot.lds should also have such fix,
>> >> since lots sections are discarded in u-boot.lds for armv8.
>> >
>> >You are correct, armv8 needs to be fixed, as well as zynq (and x86 but
>> >that's outside of my province). Anyway, that's a different problem
>> >which does not need to be addressed in this series.
>> >
>> >> >Which basically is about never discarding any section in the ELF file,
>> >> >and only copying a subset of the ELF sections into the binary file.
>> >> >
>> >> >So rather than discarding the secure relocation records, let's move
>> >> >them to another section as you had proposed, and thus change the line
>> >> >
>> >> >> +	/DISCARD/ : { *(.rel._secure*) }
>> >> >
>> >> >Into a
>> >> >
>> >> >	.rel._secure { *(.rel._secure*) }
>> >> >
>> >> >Placed right after the already present
>> >> >
>> >> >	.dynamic : { *(.dynamic*) }
>> >> 
>> >> It can not be placed after .dynamic, since the following is at front.
>> >> 87         .rel.dyn : {
>> >> 88                 *(.rel*)
>> >> 89         }
>> >> So relocation entry from secure text will first placed into .rel.dyn section.
>> >> 
>> >> If not DISCARD, then I prefer to put ".rel.secure : { *(.rel._secure*) }"
>> >> at line 55 which is wrapped by CONFIG_ARMV7_NONSEC in arch/arm/cpu/u-boot.lds.
>> >
>> >I prefer all 'unused' sections to be kept together near the end of the
>> >LDS file -- I'll add an explicit comment in the LDS about it.
>> >
>> >Besides, there no need to wrap such a section with a preprocessor
>> >conditional, as the linker will simply not emit an output section if
>> >there are no input sections at all for it; IOW, adding the '.rel._secure
>> >{ *(.rel._secure*) }' line will be binary-invariant for platforms which
>> >do not have PSCI or other secure code.
>> 
>> But ".rel._secure { *(.rel._secure*) }" can not be placed after
>> ".dynamic : { *(.dynamic*) }". Actually ".rel._secure { *(.rel._secure*) }"
>> should be placed before ".rel_dyn_start" in u-boot.lds, otherwise
>> the secure relocation entries will be placed into ".rel.dyn", but not
>> ".rel._secure".
>
>Hmm, you're correct, the linker will use the first pattern match, not
>the longest or best. :(
>
>But then, the secure reloc input section cannot go in line 55, because
>that's still inside the binary part of the image, which means it will
>waste space in there.
>
>So it's either use DISCARD at the very start of the SECTIONS (round
>line 17) or back to not generating relocatable code at all for PSCI.


I do not know how to generate non-relocatable code only for the secure text.
Since -mword-relocations and -pie are global flags, I do not know how to disable
mword-relocations when compiling PSCI and other secure text.

Is this ok for you?

diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
index 65986e8..fb2128a 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
{
+       /DISCARD/ : { *(.rel._secure*) }
        . = 0x00000000;
   
        . = ALIGN(4);

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

-- 

[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[Albert ARIBAUD]

Hello Peng,

On Wed, 21 Oct 2015 17:42:20 +0800, Peng Fan <b51431@freescale.com>
wrote:
> Hello Albert,

> I do not know how to generate non-relocatable code only for the secure text.
> Since -mword-relocations and -pie are global flags, I do not know how to disable
> mword-relocations when compiling PSCI and other secure text.

Actually, I haven't even found a compiler or gcc option to entirely
remove relocations altogether.

> Is this ok for you?
> 
> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
> index 65986e8..fb2128a 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
> {
> +       /DISCARD/ : { *(.rel._secure*) }
>         . = 0x00000000;
>    
>         . = ALIGN(4);

Functionally, it works -- I've just checked it by comparing u-boot.map
files of a mx7dsabresd build with and without the DISCARD. Not a single
symbol from the text and data section gets changed.

The only missing thing left is a comment before the DISCARD line,
explaining both why .rel._secure* input sections have to be discarded,
and also explaining why this is done it right at the start of the output
sections.

Thanks for your patience. :)

> Regards,
> Peng.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH V2 1/3] arm: discard relocation entry for secure section

[Peng Fan]

Hello Albert,

On Wed, Oct 21, 2015 at 01:42:27PM +0200, Albert ARIBAUD wrote:
>Hello Peng,
>
>On Wed, 21 Oct 2015 17:42:20 +0800, Peng Fan <b51431@freescale.com>
>wrote:
>> Hello Albert,
>
>> I do not know how to generate non-relocatable code only for the secure text.
>> Since -mword-relocations and -pie are global flags, I do not know how to disable
>> mword-relocations when compiling PSCI and other secure text.
>
>Actually, I haven't even found a compiler or gcc option to entirely
>remove relocations altogether.
>
>> Is this ok for you?
>> 
>> diff --git a/arch/arm/cpu/u-boot.lds b/arch/arm/cpu/u-boot.lds
>> index 65986e8..fb2128a 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
>> {
>> +       /DISCARD/ : { *(.rel._secure*) }
>>         . = 0x00000000;
>>    
>>         . = ALIGN(4);
>
>Functionally, it works -- I've just checked it by comparing u-boot.map
>files of a mx7dsabresd build with and without the DISCARD. Not a single
>symbol from the text and data section gets changed.
>
>The only missing thing left is a comment before the DISCARD line,
>explaining both why .rel._secure* input sections have to be discarded,
>and also explaining why this is done it right at the start of the output
>sections.

Ok. Will add the comments in V3.

Regards,
Peng.

>
>Thanks for your patience. :)
>
>> Regards,
>> Peng.
>
>Amicalement,
>-- 
>Albert.

--