alignment bugs in prom_init

alignment bugs in prom_init

[Olaf Hering]

Some G5 and pSeries models dont boot with recent kernels. The reason is
likely the casting of pointers of stack variables to u32. One example is
the prom_getprop() call in prom_init_stdout().

sp is 0x0023e784, val is at offset 120, which makes 0x0023e7fc. This
address is casted to u32, which changes it to 0x0023e7f8. The firmware
writes to the wrong addres and things go downhill very quick.

c00000000040baa8:       3b 21 00 78     addi    r25,r1,120
..
c00000000040baf4:       57 28 00 38     rlwinm  r8,r25,0,0,28
..
c00000000040bb10:       4b ff d3 3d     bl      c000000000408e4c <.call_prom>

If I remove the casts and pass the pointer as is, everything starts to
work as expected? Why is all this (u32)(unsigned long) casting in
arch/powerpc/kernel/prom_init.c required?

Does -Os vs -O2 make a difference here?

Re: alignment bugs in prom_init

[Andreas Schwab]

Olaf Hering <olh@suse.de> writes:

> sp is 0x0023e784, val is at offset 120, which makes 0x0023e7fc. This
> address is casted to u32, which changes it to 0x0023e7f8.

Since when does casting to u32 cut off the low bits?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: alignment bugs in prom_init

[Olaf Hering]

 On Fri, Mar 03, Andreas Schwab wrote:

> Olaf Hering <olh@suse.de> writes:
> 
> > sp is 0x0023e784, val is at offset 120, which makes 0x0023e7fc. This
> > address is casted to u32, which changes it to 0x0023e7f8.
> 
> Since when does casting to u32 cut off the low bits?

Way before gcc4.0 release at least.

Re: alignment bugs in prom_init

[Jerry Van Baren]

Olaf Hering wrote:
> Some G5 and pSeries models dont boot with recent kernels. The reason is
> likely the casting of pointers of stack variables to u32. One example is
> the prom_getprop() call in prom_init_stdout().
> 
> sp is 0x0023e784, val is at offset 120, which makes 0x0023e7fc. This
> address is casted to u32, which changes it to 0x0023e7f8. The firmware
> writes to the wrong addres and things go downhill very quick.
> 
> c00000000040baa8:       3b 21 00 78     addi    r25,r1,120
> ..
> c00000000040baf4:       57 28 00 38     rlwinm  r8,r25,0,0,28
> ..
> c00000000040bb10:       4b ff d3 3d     bl      c000000000408e4c <.call_prom>
> 
> If I remove the casts and pass the pointer as is, everything starts to
> work as expected? Why is all this (u32)(unsigned long) casting in
> arch/powerpc/kernel/prom_init.c required?
> 
> Does -Os vs -O2 make a difference here?
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Hi Olaf,

The casting is 8-byte aligning the address because it is a 64 bit 
variable and it is frowned on (and on some processors, fatal) to have an 
8-byte variable misaligned (not on a 8 byte boundary).

In your example above, the variable is named "sp"... the sp is suppose 
to be on a 8 byte boundary per the EABI (quoted below... note that the 
ABI requires it to be on a 16 byte boundary):
----
The Stack Frame
Unlike the SVR4 ABI, the stack pointer (GPR1) shall maintain 8-byte 
alignment, from initialization through all routine calls and dynamic 
stack space allocation.
----
In the instance above, the proper solution (but I don't know if it is a 
realistic solution :-/) is to properly align the stack pointer on a 8 
byte boundary.  I also don't know if there are other, non sp variable, 
problems.  It sounds like the prom isn't 64 bit clean.  What are our 
options to make it 64 bit clean?

Disclaimer: Yeah, I know most PPCs handle misaligned longs, but that 
doesn't make it _right_ and it definitely doesn't make it efficient.

gvb

Re: alignment bugs in prom_init

[Jerry Van Baren]

Olaf Hering wrote:
> Some G5 and pSeries models dont boot with recent kernels. The reason is
> likely the casting of pointers of stack variables to u32. One example is
> the prom_getprop() call in prom_init_stdout().
> 
> sp is 0x0023e784, val is at offset 120, which makes 0x0023e7fc. This
> address is casted to u32, which changes it to 0x0023e7f8. The firmware
> writes to the wrong addres and things go downhill very quick.
> 
> c00000000040baa8:       3b 21 00 78     addi    r25,r1,120
> ..
> c00000000040baf4:       57 28 00 38     rlwinm  r8,r25,0,0,28
> ..
> c00000000040bb10:       4b ff d3 3d     bl      c000000000408e4c <.call_prom>
> 
> If I remove the casts and pass the pointer as is, everything starts to
> work as expected? Why is all this (u32)(unsigned long) casting in
> arch/powerpc/kernel/prom_init.c required?
> 
> Does -Os vs -O2 make a difference here?

Hi Olaf, me again...

<http://gcc.gnu.org/onlinedocs/gcc-4.1.0/gcc/RS_002f6000-and-PowerPC-Options.html>

Looks like you should use -malign-natural to make it do what you want it 
to do:

-malign-natural
-malign-power
     On AIX, 32-bit Darwin, and 64-bit PowerPC GNU/Linux, the option 
-malign-natural overrides the ABI-defined alignment of larger types, 
such as floating-point doubles, on their natural size-based boundary. 
The option -malign-power instructs GCC to follow the ABI-specified 
alignment rules. GCC defaults to the standard alignment defined in the ABI.

     On 64-bit Darwin, natural alignment is the default, and 
-malign-power is not supported.

gvb

[PATCH] Workaround gcc bug #26549 which causes pointers to be truncated.

[Olaf Hering]

Workaround gcc bug #26549 which causes pointers to be truncated.

The address of variable val in prom_init_stdout is passed to prom_getprop.
prom_getprop casts the pointer to u32 and passes it to call_prom in the hope
that OpenFirmware stores something there.
But the pointer is truncated in the lower bits and the expected value is
stored somewhere else.
This gcc bug does not exist in SLES9 gcc33 toolchain, but it is present in
gcc4.0+, likely also in gcc3.4. Compiling a testcase without any -O produces
also correct code.

In my testing I had a stackpointer of 0x0023e6b4. val was at offset 120, 
wich has address 0x0023e72c. But the value passed to OF was 0x0023e728.

c00000000040b710:       3b 01 00 78     addi    r24,r1,120
...
c00000000040b754:       57 08 00 38     rlwinm  r8,r24,0,0,28
...
c00000000040b784:       80 01 00 78     lwz     r0,120(r1)
...
c00000000040b798:       90 1b 00 0c     stw     r0,12(r27)
...

simple testcase:

int f(unsigned);
void g(void)
{
	unsigned a;
	unsigned int b = (unsigned long)(void*)(&a);
	f(b);
}

asm should look like this:
   c:   38 61 00 70     addi    r3,r1,112
  10:   78 63 00 20     clrldi  r3,r3,32
  14:   48 00 00 01     bl      14 <.g+0x14>

But instead it looks like:
   c:   38 61 00 70     addi    r3,r1,112
  10:   54 63 00 36     rlwinm  r3,r3,0,0,27
  14:   48 00 00 01     bl      14 <.g+0x14>


So just uninline prom_getprop to workaround this gcc bug.

c000000000409034 <.prom_getprop>:
c000000000409054:       7c bc 2b 78     mr      r28,r5
c000000000409074:       7b 88 00 20     clrldi  r8,r28,32
...
c00000000040b29c <.prom_init>:
...
c00000000040b3d0:       3b 41 00 74     addi    r26,r1,116
...
c00000000040b3f8:       e8 82 a4 a8     ld      r4,-23384(r2)
c00000000040b3fc:       7f 45 d3 78     mr      r5,r26
c00000000040b404:       80 7d 00 04     lwz     r3,4(r29)
c00000000040b408:       38 c0 00 04     li      r6,4
c00000000040b410:       4b ff dc 25     bl      c000000000409034 <.prom_getprop>
...
c00000000040b424:       80 01 00 74     lwz     r0,116(r1)



Signed-off-by: Olaf Hering <olh@suse.de>

 arch/powerpc/kernel/prom_init.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

Index: linux-2.6.16-rc5-olh/arch/powerpc/kernel/prom_init.c
===================================================================
--- linux-2.6.16-rc5-olh.orig/arch/powerpc/kernel/prom_init.c
+++ linux-2.6.16-rc5-olh/arch/powerpc/kernel/prom_init.c
@@ -422,7 +422,8 @@ static int __init prom_next_node(phandle
 	}
 }
 
-static int inline prom_getprop(phandle node, const char *pname,
+/* do not mark as inline to work around gcc bug #26549 */
+static int __init prom_getprop(phandle node, const char *pname,
 			       void *value, size_t valuelen)
 {
 	return call_prom("getprop", 4, 1, node, ADDR(pname),

Re: alignment bugs in prom_init

[Olaf Hering]

 On Fri, Mar 03, Jerry Van Baren wrote:

> Looks like you should use -malign-natural to make it do what you want it 

Same story.

c00000000040b924:       3b 01 00 78     addi    r24,r1,120
c00000000040b968:       57 08 00 38     rlwinm  r8,r24,0,0,28
c00000000040b984:       4b ff d6 01     bl      c000000000408f84 <.call_prom>
c00000000040b998:       80 01 00 78     lwz     r0,120(r1)

,28 is even worse.

[PATCH] force stackpointer alignment in 64bit kernel

[Olaf Hering]

Fix gcc "bug" #26549 which causes pointers to be truncated.

The address of variable val in prom_init_stdout is passed to prom_getprop.
prom_getprop casts the pointer to u32 and passes it to call_prom in the hope
that OpenFirmware stores something there.
But the pointer is truncated in the lower bits and the expected value is
stored somewhere else.

In my testing I had a stackpointer of 0x0023e6b4. val was at offset 120, 
wich has address 0x0023e72c. But the value passed to OF was 0x0023e728.

c00000000040b710:       3b 01 00 78     addi    r24,r1,120
...
c00000000040b754:       57 08 00 38     rlwinm  r8,r24,0,0,28
...
c00000000040b784:       80 01 00 78     lwz     r0,120(r1)
...
c00000000040b798:       90 1b 00 0c     stw     r0,12(r27)
...

The stackpointer came from 32bit code, which appearently has different
alignment rules than 64bit code. The chain was yaboot -> zImage -> vmlinux
Force the stackpointer to be 16 byte aligned.


Signed-off-by: Olaf Hering <olh@suse.de>

 arch/powerpc/kernel/head_64.S |    2 ++
 1 files changed, 2 insertions(+)

Index: linux-2.6.16-rc5-olh/arch/powerpc/kernel/head_64.S
===================================================================
--- linux-2.6.16-rc5-olh.orig/arch/powerpc/kernel/head_64.S
+++ linux-2.6.16-rc5-olh/arch/powerpc/kernel/head_64.S
@@ -1547,6 +1547,8 @@ _STATIC(__boot_from_prom)
 	mr	r27,r7
 
 	/* Make sure we are running in 64 bits mode */
+	addi	r1,r1,16
+	rlwinm	r1,r1,0,0,28
 	bl	.enable_64b_mode
 
 	/* put a relocation offset into r3 */

Re: [PATCH] force stackpointer alignment in 64bit kernel

[Olaf Hering]

 On Fri, Mar 03, Olaf Hering wrote:

> +	addi	r1,r1,16

Guess that should be -16 ...

Re: [PATCH] force stackpointer alignment in 64bit kernel

[Segher Boessenkool]

> The stackpointer came from 32bit code, which appearently has different
> alignment rules than 64bit code. The chain was yaboot -> zImage ->  
> vmlinux
> Force the stackpointer to be 16 byte aligned.

The stack pointer is required to be 16-byte aligned when the
client program is started, on 32-bit as well.

>  	/* Make sure we are running in 64 bits mode */
> +	addi	r1,r1,16
> +	rlwinm	r1,r1,0,0,28
>  	bl	.enable_64b_mode

Not addi +16, not -16, just no addi at all...


Segher

Re: [PATCH] force stackpointer alignment in 64bit kernel

[Segher Boessenkool]

>>  	/* Make sure we are running in 64 bits mode */
>> +	addi	r1,r1,16
>> +	rlwinm	r1,r1,0,0,28
>>  	bl	.enable_64b_mode
>
> Not addi +16, not -16, just no addi at all...

Not rlwinm either, GPR1 can potentially be at >= 4GB
(of course we'd probably hit other problems then, but hey).
So please use rldicr or similar.


Segher

Re: [PATCH] force stackpointer alignment in 64bit kernel

[Olaf Hering]

 On Fri, Mar 03, Segher Boessenkool wrote:

> >The stackpointer came from 32bit code, which appearently has different
> >alignment rules than 64bit code. The chain was yaboot -> zImage ->  
> >vmlinux
> >Force the stackpointer to be 16 byte aligned.
> 
> The stack pointer is required to be 16-byte aligned when the
> client program is started, on 32-bit as well.

So the magic word is yaboot.

Re: [PATCH] force stackpointer alignment in 64bit kernel

[Olaf Hering]

 On Fri, Mar 03, Olaf Hering wrote:

> So the magic word is yaboot.

sp was 0x0023e6b4 when entering zImage.

yaboot_start has 64 bytes
yaboot_main has 304 bytes

This would mean the stackpointer for yaboot was 0x0023e824.
Will check on Monday.

Re: [PATCH] force stackpointer alignment in 64bit kernel

[Olaf Hering]

 On Fri, Mar 03, Segher Boessenkool wrote:

> > The stackpointer came from 32bit code, which appearently has different
> > alignment rules than 64bit code. The chain was yaboot -> zImage ->  
> > vmlinux
> > Force the stackpointer to be 16 byte aligned.
> 
> The stack pointer is required to be 16-byte aligned when the
> client program is started, on 32-bit as well.

See http://ozlabs.org/pipermail/linuxppc-dev/2006-March/021342.html

Either yaboot and/or zImage need to force a stack alignment, or we apply
the 2 liner.

Re: [PATCH] force stackpointer alignment in 64bit kernel

[Paul Nasrat]

On Sat, 2006-03-04 at 00:23 +0100, Olaf Hering wrote:
>  On Fri, Mar 03, Segher Boessenkool wrote:
> 
> > > The stackpointer came from 32bit code, which appearently has different
> > > alignment rules than 64bit code. The chain was yaboot -> zImage ->  
> > > vmlinux
> > > Force the stackpointer to be 16 byte aligned.
> > 
> > The stack pointer is required to be 16-byte aligned when the
> > client program is started, on 32-bit as well.
> 
> See http://ozlabs.org/pipermail/linuxppc-dev/2006-March/021342.html
> 
> Either yaboot and/or zImage need to force a stack alignment, or we apply
> the 2 liner.

Happy to look at/receive patches for yaboot.  I'll look on Monday.

Paul

Re: [PATCH] force stackpointer alignment in 64bit kernel

[Segher Boessenkool]

>> The stack pointer is required to be 16-byte aligned when the
>> client program is started, on 32-bit as well.
>
> See http://ozlabs.org/pipermail/linuxppc-dev/2006-March/021342.html
>
> Either yaboot and/or zImage need to force a stack alignment, or we  
> apply
> the 2 liner.

Yaboot should do it.  Of course, it won't hurt if the kernel will
do it itself, too...


Segher