[U-Boot] [RESEND PATCH] ARMv8: PSCI: Fix PSCI_TABLE relocation issue

[U-Boot] [RESEND PATCH] ARMv8: PSCI: Fix PSCI_TABLE relocation issue

[Lars Povlsen]

This fixes relaction isses with the PSCI_TABLE entries in
the psci_32_table and psci_64_table.

When using 32-bit adress pointers relocation was not being applied to
the tables, causing PSCI handlers to point to the un-relocated code
area. By using 64-bit data relocation is properly applied. The
handlers are thus in the "secure data" area, which is protected by
/memreserve/ in the FDT.

Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
---
 arch/arm/cpu/armv8/psci.S | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/arm/cpu/armv8/psci.S b/arch/arm/cpu/armv8/psci.S
index 358df8fee9..b4568cf053 100644
--- a/arch/arm/cpu/armv8/psci.S
+++ b/arch/arm/cpu/armv8/psci.S
@@ -19,8 +19,8 @@
 
 /* PSCI function and ID table definition*/
 #define PSCI_TABLE(__id, __fn) \
-	.word __id; \
-	.word __fn
+	.quad __id; \
+	.quad __fn
 
 .pushsection ._secure.text, "ax"
 
@@ -132,16 +132,15 @@ PSCI_TABLE(0, 0)
 /* Caller must put PSCI function-ID table base in x9 */
 handle_psci:
 	psci_enter
-1:	ldr x10, [x9]			/* Load PSCI function table */
-	ubfx x11, x10, #32, #32
-	ubfx x10, x10, #0, #32
+1:	ldr	x10, [x9]		/* Load PSCI function table */
 	cbz	x10, 3f			/* If reach the end, bail out */
 	cmp	x10, x0
 	b.eq	2f			/* PSCI function found */
-	add x9, x9, #8			/* If not match, try next entry */
+	add x9, x9, #16			/* If not match, try next entry */
 	b	1b
 
-2:	blr	x11			/* Call PSCI function */
+2:	ldr	x11, [x9, #8]		/* Load PSCI function */
+	blr	x11			/* Call PSCI function */
 	psci_return
 
 3:	mov	x0, #ARM_PSCI_RET_NI
-- 
2.21.0

[U-Boot] [RESEND PATCH] ARMv8: PSCI: Fix PSCI_TABLE relocation issue

[Ang, Chee Hong]

On Thu, 2019-04-04 at 14:38 +0200, Lars Povlsen wrote:
> This fixes relaction isses with the PSCI_TABLE entries in
> the psci_32_table and psci_64_table.
> 
> When using 32-bit adress pointers relocation was not being applied to
> the tables, causing PSCI handlers to point to the un-relocated code
> area. By using 64-bit data relocation is properly applied. The
> handlers are thus in the "secure data" area, which is protected by
> /memreserve/ in the FDT.
> 
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> ---
>  arch/arm/cpu/armv8/psci.S | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv8/psci.S b/arch/arm/cpu/armv8/psci.S
> index 358df8fee9..b4568cf053 100644
> --- a/arch/arm/cpu/armv8/psci.S
> +++ b/arch/arm/cpu/armv8/psci.S
> @@ -19,8 +19,8 @@
>  
>  /* PSCI function and ID table definition*/
>  #define PSCI_TABLE(__id, __fn) \
> -	.word __id; \
> -	.word __fn
> +	.quad __id; \
> +	.quad __fn
>  
>  .pushsection ._secure.text, "ax"
>  
> @@ -132,16 +132,15 @@ PSCI_TABLE(0, 0)
>  /* Caller must put PSCI function-ID table base in x9 */
>  handle_psci:
>  	psci_enter
> -1:	ldr x10, [x9]			/* Load PSCI function
> table */
> -	ubfx x11, x10, #32, #32
> -	ubfx x10, x10, #0, #32
> +1:	ldr	x10, [x9]		/* Load PSCI function
> table */
>  	cbz	x10, 3f			/* If reach the
> end, bail out */
>  	cmp	x10, x0
>  	b.eq	2f			/* PSCI function found
> */
> -	add x9, x9, #8			/* If not match, try
> next entry */
> +	add x9, x9, #16			/* If not match, try
> next entry */
>  	b	1b
>  
> -2:	blr	x11			/* Call PSCI
> function */
> +2:	ldr	x11, [x9, #8]		/* Load PSCI
> function */
> +	blr	x11			/* Call PSCI function
> */
>  	psci_return
>  
>  3:	mov	x0, #ARM_PSCI_RET_NI

Hmmm...I presumed you made these changes because your relocation
address for "secure" section is beyond 32bits (4GB). How your page
table for MMU is being setup ? Why do you need such large address space
(beyond 4GB) ?

[U-Boot] [RESEND PATCH] ARMv8: PSCI: Fix PSCI_TABLE relocation issue

[Lars.Povlsen at microchip.com]

> From: Ang, Chee Hong <chee.hong.ang@intel.com>
> Sent: Monday, April 8, 2019 05:10
> To: Lars Povlsen - M31675 <Lars.Povlsen@microchip.com>;
> trini at konsulko.com; u-boot at lists.denx.de; macro.wave.z at gmail.com;
> albert.u.boot at aribaud.net; york.sun at nxp.com
> Subject: Re: [RESEND PATCH] ARMv8: PSCI: Fix PSCI_TABLE relocation issue
> 
> On Thu, 2019-04-04 at 14:38 +0200, Lars Povlsen wrote:
> > This fixes relaction isses with the PSCI_TABLE entries in
> > the psci_32_table and psci_64_table.
> >
> > When using 32-bit adress pointers relocation was not being applied to
> > the tables, causing PSCI handlers to point to the un-relocated code
> > area. By using 64-bit data relocation is properly applied. The
> > handlers are thus in the "secure data" area, which is protected by
> > /memreserve/ in the FDT.
> >
> > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> > ---
> >  arch/arm/cpu/armv8/psci.S | 13 ++++++-------
> >  1 file changed, 6 insertions(+), 7 deletions(-)
> >
> > diff --git a/arch/arm/cpu/armv8/psci.S b/arch/arm/cpu/armv8/psci.S
> > index 358df8fee9..b4568cf053 100644
> > --- a/arch/arm/cpu/armv8/psci.S
> > +++ b/arch/arm/cpu/armv8/psci.S
> > @@ -19,8 +19,8 @@
> >
> >  /* PSCI function and ID table definition*/
> >  #define PSCI_TABLE(__id, __fn) \
> > -	.word __id; \
> > -	.word __fn
> > +	.quad __id; \
> > +	.quad __fn
> >
> >  .pushsection ._secure.text, "ax"
> >
> > @@ -132,16 +132,15 @@ PSCI_TABLE(0, 0)
> >  /* Caller must put PSCI function-ID table base in x9 */
> >  handle_psci:
> >  	psci_enter
> > -1:	ldr x10, [x9]			/* Load PSCI function
> > table */
> > -	ubfx x11, x10, #32, #32
> > -	ubfx x10, x10, #0, #32
> > +1:	ldr	x10, [x9]		/* Load PSCI function
> > table */
> >  	cbz	x10, 3f			/* If reach the
> > end, bail out */
> >  	cmp	x10, x0
> >  	b.eq	2f			/* PSCI function found
> > */
> > -	add x9, x9, #8			/* If not match, try
> > next entry */
> > +	add x9, x9, #16			/* If not match, try
> > next entry */
> >  	b	1b
> >
> > -2:	blr	x11			/* Call PSCI
> > function */
> > +2:	ldr	x11, [x9, #8]		/* Load PSCI
> > function */
> > +	blr	x11			/* Call PSCI function
> > */
> >  	psci_return
> >
> >  3:	mov	x0, #ARM_PSCI_RET_NI
> 
> Hmmm...I presumed you made these changes because your relocation
> address for "secure" section is beyond 32bits (4GB). How your page
> table for MMU is being setup ? Why do you need such large address space
> (beyond 4GB) ?

No, as I tried to describe in the log message, the relocation was simply
not being applied. Changing the offsets to 64-bit fixed this. The .text base
was 0x80000000 and the relocation was done in a 2Gb window (so somewhere ~ 0xfff.....)

Never the less, I assume a 64-bit target should not be constrained to 32-bit addresses?

When debugging the code, I noticed my debugger was able to match the original symbols
even though relocation was done. That’s how I became aware. I could see that the vector table
and the first level code *was* relocated, but the individual handlers not.

---Lars

[U-Boot] [RESEND PATCH] ARMv8: PSCI: Fix PSCI_TABLE relocation issue

[Ang, Chee Hong]

On Mon, 2019-04-08 at 07:27 +0000, Lars.Povlsen at microchip.com wrote:
> > 
> > From: Ang, Chee Hong <chee.hong.ang@intel.com>
> > Sent: Monday, April 8, 2019 05:10
> > To: Lars Povlsen - M31675 <Lars.Povlsen@microchip.com>;
> > trini at konsulko.com; u-boot at lists.denx.de; macro.wave.z at gmail.com;
> > albert.u.boot at aribaud.net; york.sun at nxp.com
> > Subject: Re: [RESEND PATCH] ARMv8: PSCI: Fix PSCI_TABLE relocation
> > issue
> > 
> > On Thu, 2019-04-04 at 14:38 +0200, Lars Povlsen wrote:
> > > 
> > > This fixes relaction isses with the PSCI_TABLE entries in
> > > the psci_32_table and psci_64_table.
> > > 
> > > When using 32-bit adress pointers relocation was not being
> > > applied to
> > > the tables, causing PSCI handlers to point to the un-relocated
> > > code
> > > area. By using 64-bit data relocation is properly applied. The
> > > handlers are thus in the "secure data" area, which is protected
> > > by
> > > /memreserve/ in the FDT.
> > > 
> > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> > > ---
> > >  arch/arm/cpu/armv8/psci.S | 13 ++++++-------
> > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/arch/arm/cpu/armv8/psci.S
> > > b/arch/arm/cpu/armv8/psci.S
> > > index 358df8fee9..b4568cf053 100644
> > > --- a/arch/arm/cpu/armv8/psci.S
> > > +++ b/arch/arm/cpu/armv8/psci.S
> > > @@ -19,8 +19,8 @@
> > > 
> > >  /* PSCI function and ID table definition*/
> > >  #define PSCI_TABLE(__id, __fn) \
> > > -	.word __id; \
> > > -	.word __fn
> > > +	.quad __id; \
> > > +	.quad __fn
> > > 
> > >  .pushsection ._secure.text, "ax"
> > > 
> > > @@ -132,16 +132,15 @@ PSCI_TABLE(0, 0)
> > >  /* Caller must put PSCI function-ID table base in x9 */
> > >  handle_psci:
> > >  	psci_enter
> > > -1:	ldr x10, [x9]			/* Load PSCI
> > > function
> > > table */
> > > -	ubfx x11, x10, #32, #32
> > > -	ubfx x10, x10, #0, #32
> > > +1:	ldr	x10, [x9]		/* Load PSCI
> > > function
> > > table */
> > >  	cbz	x10, 3f			/* If reach
> > > the
> > > end, bail out */
> > >  	cmp	x10, x0
> > >  	b.eq	2f			/* PSCI function
> > > found
> > > */
> > > -	add x9, x9, #8			/* If not match,
> > > try
> > > next entry */
> > > +	add x9, x9, #16			/* If not match,
> > > try
> > > next entry */
> > >  	b	1b
> > > 
> > > -2:	blr	x11			/* Call PSCI
> > > function */
> > > +2:	ldr	x11, [x9, #8]		/* Load PSCI
> > > function */
> > > +	blr	x11			/* Call PSCI
> > > function
> > > */
> > >  	psci_return
> > > 
> > >  3:	mov	x0, #ARM_PSCI_RET_NI
> > Hmmm...I presumed you made these changes because your relocation
> > address for "secure" section is beyond 32bits (4GB). How your page
> > table for MMU is being setup ? Why do you need such large address
> > space
> > (beyond 4GB) ?
> No, as I tried to describe in the log message, the relocation was
> simply
> not being applied. Changing the offsets to 64-bit fixed this. The
> .text base
> was 0x80000000 and the relocation was done in a 2Gb window (so
> somewhere ~ 0xfff.....)
> 
> Never the less, I assume a 64-bit target should not be constrained to
> 32-bit addresses?
> 
> When debugging the code, I noticed my debugger was able to match the
> original symbols
> even though relocation was done. That’s how I became aware. I could
> see that the vector table
> and the first level code *was* relocated, but the individual handlers
> not.
> ---Lars
CONFIG_ARMV8_SECURE_BASE defines the target relocation address for the
PSCI code and the table. I just checked the PSCI handler addresses in
the PSCI table entry (_psci_32_table & _psci_64_table) of my uboot,
they point to the relocated address without using the 64bits size.

[U-Boot] [RESEND PATCH] ARMv8: PSCI: Fix PSCI_TABLE relocation issue

[Lars.Povlsen at microchip.com]

> From: Ang, Chee Hong <chee.hong.ang@intel.com>
> Sent: Monday, April 8, 2019 11:00
> To: Lars Povlsen - M31675 <Lars.Povlsen@microchip.com>;
> trini at konsulko.com; u-boot at lists.denx.de; macro.wave.z at gmail.com;
> albert.u.boot at aribaud.net; york.sun at nxp.com
> Subject: Re: [RESEND PATCH] ARMv8: PSCI: Fix PSCI_TABLE relocation issue
> 
> On Mon, 2019-04-08 at 07:27 +0000, Lars.Povlsen at microchip.com wrote:
> > >
> > > From: Ang, Chee Hong <chee.hong.ang@intel.com>
> > > Sent: Monday, April 8, 2019 05:10
> > > To: Lars Povlsen - M31675 <Lars.Povlsen@microchip.com>;
> > > trini at konsulko.com; u-boot at lists.denx.de; macro.wave.z at gmail.com;
> > > albert.u.boot at aribaud.net; york.sun at nxp.com
> > > Subject: Re: [RESEND PATCH] ARMv8: PSCI: Fix PSCI_TABLE relocation
> > > issue
> > >
> > > On Thu, 2019-04-04 at 14:38 +0200, Lars Povlsen wrote:
> > > >
> > > > This fixes relaction isses with the PSCI_TABLE entries in
> > > > the psci_32_table and psci_64_table.
> > > >
> > > > When using 32-bit adress pointers relocation was not being
> > > > applied to
> > > > the tables, causing PSCI handlers to point to the un-relocated
> > > > code
> > > > area. By using 64-bit data relocation is properly applied. The
> > > > handlers are thus in the "secure data" area, which is protected
> > > > by
> > > > /memreserve/ in the FDT.
> > > >
> > > > Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
> > > > ---
> > > >  arch/arm/cpu/armv8/psci.S | 13 ++++++-------
> > > >  1 file changed, 6 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/arch/arm/cpu/armv8/psci.S
> > > > b/arch/arm/cpu/armv8/psci.S
> > > > index 358df8fee9..b4568cf053 100644
> > > > --- a/arch/arm/cpu/armv8/psci.S
> > > > +++ b/arch/arm/cpu/armv8/psci.S
> > > > @@ -19,8 +19,8 @@
> > > >
> > > >  /* PSCI function and ID table definition*/
> > > >  #define PSCI_TABLE(__id, __fn) \
> > > > -	.word __id; \
> > > > -	.word __fn
> > > > +	.quad __id; \
> > > > +	.quad __fn
> > > >
> > > >  .pushsection ._secure.text, "ax"
> > > >
> > > > @@ -132,16 +132,15 @@ PSCI_TABLE(0, 0)
> > > >  /* Caller must put PSCI function-ID table base in x9 */
> > > >  handle_psci:
> > > >  	psci_enter
> > > > -1:	ldr x10, [x9]			/* Load PSCI
> > > > function
> > > > table */
> > > > -	ubfx x11, x10, #32, #32
> > > > -	ubfx x10, x10, #0, #32
> > > > +1:	ldr	x10, [x9]		/* Load PSCI
> > > > function
> > > > table */
> > > >  	cbz	x10, 3f			/* If reach
> > > > the
> > > > end, bail out */
> > > >  	cmp	x10, x0
> > > >  	b.eq	2f			/* PSCI function
> > > > found
> > > > */
> > > > -	add x9, x9, #8			/* If not match,
> > > > try
> > > > next entry */
> > > > +	add x9, x9, #16			/* If not match,
> > > > try
> > > > next entry */
> > > >  	b	1b
> > > >
> > > > -2:	blr	x11			/* Call PSCI
> > > > function */
> > > > +2:	ldr	x11, [x9, #8]		/* Load PSCI
> > > > function */
> > > > +	blr	x11			/* Call PSCI
> > > > function
> > > > */
> > > >  	psci_return
> > > >
> > > >  3:	mov	x0, #ARM_PSCI_RET_NI
> > > Hmmm...I presumed you made these changes because your relocation
> > > address for "secure" section is beyond 32bits (4GB). How your page
> > > table for MMU is being setup ? Why do you need such large address
> > > space
> > > (beyond 4GB) ?
> > No, as I tried to describe in the log message, the relocation was
> > simply
> > not being applied. Changing the offsets to 64-bit fixed this. The
> > .text base
> > was 0x80000000 and the relocation was done in a 2Gb window (so
> > somewhere ~ 0xfff.....)
> >
> > Never the less, I assume a 64-bit target should not be constrained to
> > 32-bit addresses?
> >
> > When debugging the code, I noticed my debugger was able to match the
> > original symbols
> > even though relocation was done. That’s how I became aware. I could
> > see that the vector table
> > and the first level code *was* relocated, but the individual handlers
> > not.
> > ---Lars
> CONFIG_ARMV8_SECURE_BASE defines the target relocation address for the
> PSCI code and the table. I just checked the PSCI handler addresses in
> the PSCI table entry (_psci_32_table & _psci_64_table) of my uboot,
> they point to the relocated address without using the 64bits size.

CONFIG_ARMV8_SECURE_BASE depend on SYS_HAS_ARMV8_SECURE_BASE, which is
not set on this platform. Might this be the source of the problem?
In this case, I would assume that the "secure data" just stays with the
relocated U-boot, and gets carved out with a fdt_add_mem_rsv when
booting Linux.

For what it matters, I have changed to using the spintable PSCI variant,
so I'm not really depending on this patch. But it looks like there is a
problem, at least in the configuration I used.


---Lars

[U-Boot] [U-Boot, RESEND] ARMv8: PSCI: Fix PSCI_TABLE relocation issue

[Tom Rini]

On Thu, Apr 04, 2019 at 02:38:50PM +0200, Lars Povlsen wrote:

> This fixes relaction isses with the PSCI_TABLE entries in
> the psci_32_table and psci_64_table.
> 
> When using 32-bit adress pointers relocation was not being applied to
> the tables, causing PSCI handlers to point to the un-relocated code
> area. By using 64-bit data relocation is properly applied. The
> handlers are thus in the "secure data" area, which is protected by
> /memreserve/ in the FDT.
> 
> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190424/bce10c86/attachment.sig>