help with powerpc asm (moving grub_ieee1275_entry_fn initialization to crt0.S)

help with powerpc asm (moving grub_ieee1275_entry_fn initialization to crt0.S)

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 706 bytes --]


Hi,

For the i386-ieee1275 port, I'd need to move grub_ieee1275_entry_fn
initialization in powerpc from cmain.c to crt0.S, so that code in cmain.c
can be shared with i386.

The problem is I have no clue about powerpc assembler.  On i386, this is
just done with a "movl %eax, EXT_C(grub_ieee1275_entry_fn)" but I don't know
the equivalent.  Furthermore, the code in crt0.S seems to be pushing arguments
(up to 3 according to cmain.c) in a loop, which got me really confused.

Anyone who is familiar with powerpc assembler could have a look?  See attached
patch.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

[-- Attachment #2: grub_ieee1275_entry_fn.diff --]
[-- Type: text/x-diff, Size: 977 bytes --]

diff -ur grub2/kern/powerpc/ieee1275/cmain.c grub2.powerpc/kern/powerpc/ieee1275/cmain.c
--- grub2/kern/powerpc/ieee1275/cmain.c	2007-12-30 09:52:05.000000000 +0100
+++ grub2.powerpc/kern/powerpc/ieee1275/cmain.c	2008-01-13 20:01:48.000000000 +0100
@@ -102,12 +102,9 @@
     }
 }
 
-void cmain (uint32_t r3, uint32_t r4, uint32_t r5);
 void
-cmain (UNUSED uint32_t r3, UNUSED uint32_t r4, uint32_t r5)
+cmain (void)
 {
-  grub_ieee1275_entry_fn = (int (*)(void *)) r5;
-
   grub_ieee1275_finddevice ("/chosen", &grub_ieee1275_chosen);
 
   grub_ieee1275_find_options ();
diff -ur grub2/kern/powerpc/ieee1275/crt0.S grub2.powerpc/kern/powerpc/ieee1275/crt0.S
--- grub2/kern/powerpc/ieee1275/crt0.S	2007-07-22 01:32:27.000000000 +0200
+++ grub2.powerpc/kern/powerpc/ieee1275/crt0.S	2008-01-13 20:01:34.000000000 +0100
@@ -38,5 +38,7 @@
 2:	stwu	2, 4(6) /* We know r2 is already 0 from above.  */
 	bdnz	2b
 
+	/* initialize EXT_C(grub_ieee1275_entry_fn) */
+
 	bl	cmain
 1:	b	1b

Re: help with powerpc asm (moving grub_ieee1275_entry_fn initialization to crt0.S)

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]


Here's an incomplete (missing powerpc & sparc) version of the patch that
would sanitize this function call.

In the meantime i386 just puts the value in %ecx (third argument) to match
what powerpc is doing.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call, if you are unable to speak?
(as seen on /.)

[-- Attachment #2: ieee1275_statup.diff --]
[-- Type: text/x-diff, Size: 977 bytes --]

diff -urp grub2/kern/i386/ieee1275/startup.S ieee1275_statup/kern/i386/ieee1275/startup.S
--- grub2/kern/i386/ieee1275/startup.S	2008-01-15 21:05:44.000000000 +0100
+++ ieee1275_statup/kern/i386/ieee1275/startup.S	2008-01-15 21:06:18.000000000 +0100
@@ -34,5 +34,5 @@
 
 start:
 _start:
-	movl %eax, %ecx
+	movl %eax, EXT_C(grub_ieee1275_entry_fn)
 	jmp EXT_C(cmain)
diff -urp grub2/kern/powerpc/ieee1275/cmain.c ieee1275_statup/kern/powerpc/ieee1275/cmain.c
--- grub2/kern/powerpc/ieee1275/cmain.c	2008-01-15 17:13:55.000000000 +0100
+++ ieee1275_statup/kern/powerpc/ieee1275/cmain.c	2008-01-15 21:06:38.000000000 +0100
@@ -102,12 +102,9 @@ grub_ieee1275_find_options (void)
     }
 }
 
-void cmain (uint32_t r3, uint32_t r4, uint32_t r5);
 void
-cmain (UNUSED uint32_t r3, UNUSED uint32_t r4, uint32_t r5)
+cmain (void)
 {
-  grub_ieee1275_entry_fn = (int (*)(void *)) r5;
-
   grub_ieee1275_finddevice ("/chosen", &grub_ieee1275_chosen);
 
   grub_ieee1275_find_options ();

Re: help with powerpc asm (moving grub_ieee1275_entry_fn initialization to crt0.S)

[Pavel Roskin]

Quoting Robert Millan <rmh@aybabtu.com>:

> Here's an incomplete (missing powerpc & sparc) version of the patch that
> would sanitize this function call.

I don't see why it needs to be done across the board.  Maybe you have  
further simplifications in mind?  Anyway, it's a good idea not to rely  
on C calling conventions on any platform.

Here's the patch for PowerPC.  It has been tested both in qemu and on  
real hardware.  I have verified that the value of  
grub_ieee1275_entry_fn doesn't change.

By the way, please don't remove the declaration of cmain().  It may be  
useful to prevent a gcc warning (missing declaration).  cmain() is  
really an rare case that doesn't need a declaration, since it's not  
called by any C code, but gcc doesn't know that.

ChangeLog:

        * kern/powerpc/ieee1275/cmain.c (cmain): Don't take any arguments.
        * kern/powerpc/ieee1275/crt0.S: Store r5 in grub_ieee1275_entry_fn,
        don't rely on cmain() doing it.

diff --git a/kern/powerpc/ieee1275/cmain.c b/kern/powerpc/ieee1275/cmain.c
index 5d4b0de..30dfe1c 100644
--- a/kern/powerpc/ieee1275/cmain.c
+++ b/kern/powerpc/ieee1275/cmain.c
@@ -114,12 +114,10 @@ grub_ieee1275_find_options (void)
    }
  }

-void cmain (uint32_t r3, uint32_t r4, uint32_t r5);
+void cmain (void);
  void
-cmain (UNUSED uint32_t r3, UNUSED uint32_t r4, uint32_t r5)
+cmain (void)
  {
-  grub_ieee1275_entry_fn = (int (*)(void *)) r5;
-
    grub_ieee1275_finddevice ("/chosen", &grub_ieee1275_chosen);

    grub_ieee1275_find_options ();
diff --git a/kern/powerpc/ieee1275/crt0.S b/kern/powerpc/ieee1275/crt0.S
index f295dea..f6545fb 100644
--- a/kern/powerpc/ieee1275/crt0.S
+++ b/kern/powerpc/ieee1275/crt0.S
@@ -38,5 +38,9 @@ _start:
  2:	stwu	2, 4(6) /* We know r2 is already 0 from above.  */
  	bdnz	2b

+	/* Store r5 in grub_ieee1275_entry_fn */
+	lis	9, grub_ieee1275_entry_fn@ha
+	stw	5, grub_ieee1275_entry_fn@l(9)
+
  	bl	cmain
  1:	b	1b

-- 
Regards,
Pavel Roskin

Re: help with powerpc asm (moving grub_ieee1275_entry_fn initialization to crt0.S)

[Robert Millan]

On Thu, Jan 17, 2008 at 02:54:33AM -0500, Pavel Roskin wrote:
> Quoting Robert Millan <rmh@aybabtu.com>:
> 
> >Here's an incomplete (missing powerpc & sparc) version of the patch that
> >would sanitize this function call.
> 
> I don't see why it needs to be done across the board.  Maybe you have  
> further simplifications in mind?

What do you mean?  The problem I find is that forcing this to be the third
parameter of cmain makes it look like cmain has been adapted to accomodate
for this particular cpu.

Another option would be to have just one param.  This would be the cleanest
on i386, since the call from OFW already uses -mregparm=1 for the callback
address, but I don't know about powerpc.

> By the way, please don't remove the declaration of cmain().  It may be  
> useful to prevent a gcc warning (missing declaration).  cmain() is  
> really an rare case that doesn't need a declaration, since it's not  
> called by any C code, but gcc doesn't know that.

Ah yes.  Right.

> --- a/kern/powerpc/ieee1275/crt0.S
> +++ b/kern/powerpc/ieee1275/crt0.S
> @@ -38,5 +38,9 @@ _start:
>  2:	stwu	2, 4(6) /* We know r2 is already 0 from above.  */
>  	bdnz	2b
> 
> +	/* Store r5 in grub_ieee1275_entry_fn */
> +	lis	9, grub_ieee1275_entry_fn@ha
> +	stw	5, grub_ieee1275_entry_fn@l(9)

I thought you'd also need to remove stuff.  Isn't there code above this that
pushes the 3 parameters onto the stack?

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

Re: help with powerpc asm (moving grub_ieee1275_entry_fn initialization to crt0.S)

[Pavel Roskin]

On Thu, 2008-01-17 at 13:17 +0100, Robert Millan wrote:
> On Thu, Jan 17, 2008 at 02:54:33AM -0500, Pavel Roskin wrote:
> > Quoting Robert Millan <rmh@aybabtu.com>:
> > 
> > >Here's an incomplete (missing powerpc & sparc) version of the patch that
> > >would sanitize this function call.
> > 
> > I don't see why it needs to be done across the board.  Maybe you have  
> > further simplifications in mind?
> 
> What do you mean?  The problem I find is that forcing this to be the third
> parameter of cmain makes it look like cmain has been adapted to accomodate
> for this particular cpu.

Oh, I didn't realize that kern/powerpc/ieee1275/cmain.c is actually used
by the i386-ieee1275 build.  That's misleading!  Perhaps it should be
moved to kern/ieee1275/

Anyway, Sparc code doesn't use cmain() in any way.  It starts
immediately with the function _start() written in C.  So you shouldn't
worry about breaking Sparc.

Please feel free to integrate the PowerPC part of my patch into your
patch, as it indeed needs to be committed at once.

> Another option would be to have just one param.  This would be the cleanest
> on i386, since the call from OFW already uses -mregparm=1 for the callback
> address, but I don't know about powerpc.

I would prefer that we don't choose global CFLAGS to adjust one
function.  There is a "regparm" attribute that can be used on cmain() on
i386 architecture.

But if you want to keep cmain() shared between architectures, then the
cleanest approach would be to take care of grub_ieee1275_entry_fn before
cmain() is called.  It doesn't have to be in assembly.  Sparc can still
use its _start function to set grub_ieee1275_entry_fn and then call
cmain().

> > --- a/kern/powerpc/ieee1275/crt0.S
> > +++ b/kern/powerpc/ieee1275/crt0.S
> > @@ -38,5 +38,9 @@ _start:
> >  2:	stwu	2, 4(6) /* We know r2 is already 0 from above.  */
> >  	bdnz	2b
> > 
> > +	/* Store r5 in grub_ieee1275_entry_fn */
> > +	lis	9, grub_ieee1275_entry_fn@ha
> > +	stw	5, grub_ieee1275_entry_fn@l(9)
> 
> I thought you'd also need to remove stuff.  Isn't there code above this that
> pushes the 3 parameters onto the stack?

Arguments are transmitted in registers r3, r4 and r5.  Nothing is passed
on the stack.  r5 has the data we want to be in grub_ieee1275_entry_fn.

The first instruction takes the high 16 bits of the
grub_ieee1275_entry_fn address and put is to register 9.  Register 9 is
the favorite choice for memory operations, as it has no special purpose
and doesn't need to be preserved.

The second instruction places the contents of register 5 into an address
pointed to by the register 9 plus the lower 16 bits of the 
grub_ieee1275_entry_fn address.

A 32-bit constant cannot be loaded in one operation, since all PowerPC
commands are 32-bit, and it simply won't fit one command.

-- 
Regards,
Pavel Roskin

Re: help with powerpc asm (moving grub_ieee1275_entry_fn initialization to crt0.S)

[Robert Millan]

[-- Attachment #1: Type: text/plain, Size: 2873 bytes --]

On Thu, Jan 17, 2008 at 12:30:19PM -0500, Pavel Roskin wrote:
> 
> Oh, I didn't realize that kern/powerpc/ieee1275/cmain.c is actually used
> by the i386-ieee1275 build.  That's misleading!  Perhaps it should be
> moved to kern/ieee1275/

Yes.  I had this in mind, but I think it's better to wait untill the i386
port is complete before we start moving files.

> Anyway, Sparc code doesn't use cmain() in any way.  It starts
> immediately with the function _start() written in C.  So you shouldn't
> worry about breaking Sparc.

Ah, I see.  I didn't expect this.

> Please feel free to integrate the PowerPC part of my patch into your
> patch, as it indeed needs to be committed at once.

Ok.  I'm attaching the complete patch.

> > Another option would be to have just one param.  This would be the cleanest
> > on i386, since the call from OFW already uses -mregparm=1 for the callback
> > address, but I don't know about powerpc.
> 
> I would prefer that we don't choose global CFLAGS to adjust one
> function.  There is a "regparm" attribute that can be used on cmain() on
> i386 architecture.

The CFLAGS for i386-ieee1275 are already set to regparm.  This makes it easier
to interact with OFW in both ways, since call backs also use %eax as first
(and only) parameter.

In fact, our current code seems to assume that grub_ieee1275_entry_fn() always
has the same calling conventions than GRUB.  Because of this, I thought it'd
be consistent to assume the same for our _start routine.

However, powerpc and sparc64 seem to disagree with i386 on which is the
parameter that ought to be used to initialize grub_ieee1275_entry_fn.  I
suppose it's better to move this out of cmain, then..

> But if you want to keep cmain() shared between architectures, then the
> cleanest approach would be to take care of grub_ieee1275_entry_fn before
> cmain() is called.  It doesn't have to be in assembly.  Sparc can still
> use its _start function to set grub_ieee1275_entry_fn and then call
> cmain().

Ok.  That sounds good.

> Arguments are transmitted in registers r3, r4 and r5.  Nothing is passed
> on the stack.  r5 has the data we want to be in grub_ieee1275_entry_fn.
> 
> The first instruction takes the high 16 bits of the
> grub_ieee1275_entry_fn address and put is to register 9.  Register 9 is
> the favorite choice for memory operations, as it has no special purpose
> and doesn't need to be preserved.
> 
> The second instruction places the contents of register 5 into an address
> pointed to by the register 9 plus the lower 16 bits of the 
> grub_ieee1275_entry_fn address.
> 
> A 32-bit constant cannot be loaded in one operation, since all PowerPC
> commands are 32-bit, and it simply won't fit one command.

I see..

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)

[-- Attachment #2: cmain.diff --]
[-- Type: text/x-diff, Size: 2516 bytes --]


	PowerPC changes provided by Pavel Roskin.

	* kern/powerpc/ieee1275/cmain.c (cmain): Don't take any arguments.
	* kern/powerpc/ieee1275/crt0.S: Store r5 in grub_ieee1275_entry_fn,
	don't rely on cmain() doing it.
	* kern/i386/ieee1275/startup.S (_start): Store %eax in
	grub_ieee1275_entry_fn, don't rely on cmain() doing it.

diff -ur grub2/kern/i386/ieee1275/startup.S cmain/kern/i386/ieee1275/startup.S
--- grub2/kern/i386/ieee1275/startup.S	2008-01-15 21:05:44.000000000 +0100
+++ cmain/kern/i386/ieee1275/startup.S	2008-01-17 20:05:51.000000000 +0100
@@ -34,5 +34,5 @@
 
 start:
 _start:
-	movl %eax, %ecx
+	movl %eax, EXT_C(grub_ieee1275_entry_fn)
 	jmp EXT_C(cmain)
diff -ur grub2/kern/powerpc/ieee1275/cmain.c cmain/kern/powerpc/ieee1275/cmain.c
--- grub2/kern/powerpc/ieee1275/cmain.c	2008-01-15 17:14:32.000000000 +0100
+++ cmain/kern/powerpc/ieee1275/cmain.c	2008-01-17 20:05:11.000000000 +0100
@@ -1,7 +1,7 @@
 /* cmain.c - Startup code for the PowerPC.  */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2003,2004,2005,2006,2007  Free Software Foundation, Inc.
+ *  Copyright (C) 2003,2004,2005,2006,2007,2008  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -102,12 +102,10 @@
     }
 }
 
-void cmain (uint32_t r3, uint32_t r4, uint32_t r5);
+void cmain (void);
 void
-cmain (UNUSED uint32_t r3, UNUSED uint32_t r4, uint32_t r5)
+cmain (void)
 {
-  grub_ieee1275_entry_fn = (int (*)(void *)) r5;
-
   grub_ieee1275_finddevice ("/chosen", &grub_ieee1275_chosen);
 
   grub_ieee1275_find_options ();
diff -ur grub2/kern/powerpc/ieee1275/crt0.S cmain/kern/powerpc/ieee1275/crt0.S
--- grub2/kern/powerpc/ieee1275/crt0.S	2007-07-22 01:32:27.000000000 +0200
+++ cmain/kern/powerpc/ieee1275/crt0.S	2008-01-17 20:05:04.000000000 +0100
@@ -1,7 +1,7 @@
 /* crt0.S - Startup code for the PowerPC.  */
 /*
  *  GRUB  --  GRand Unified Bootloader
- *  Copyright (C) 2003,2004,2005,2007  Free Software Foundation, Inc.
+ *  Copyright (C) 2003,2004,2005,2007,2008  Free Software Foundation, Inc.
  *
  *  GRUB is free software: you can redistribute it and/or modify
  *  it under the terms of the GNU General Public License as published by
@@ -38,5 +38,9 @@
 2:	stwu	2, 4(6) /* We know r2 is already 0 from above.  */
 	bdnz	2b
 
+	/* Store r5 in grub_ieee1275_entry_fn.  */
+	lis	9, grub_ieee1275_entry_fn@ha
+	stw	5, grub_ieee1275_entry_fn@l(9)
+
 	bl	cmain
 1:	b	1b

Re: help with powerpc asm (moving grub_ieee1275_entry_fn initialization to crt0.S)

[Pavel Roskin]

On Thu, 2008-01-17 at 20:11 +0100, Robert Millan wrote:

> > Please feel free to integrate the PowerPC part of my patch into your
> > patch, as it indeed needs to be committed at once.
> 
> Ok.  I'm attaching the complete patch.

Fine with me.

-- 
Regards,
Pavel Roskin

Re: help with powerpc asm (moving grub_ieee1275_entry_fn initialization to crt0.S)

[Robert Millan]

On Thu, Jan 17, 2008 at 03:08:24PM -0500, Pavel Roskin wrote:
> On Thu, 2008-01-17 at 20:11 +0100, Robert Millan wrote:
> 
> > > Please feel free to integrate the PowerPC part of my patch into your
> > > patch, as it indeed needs to be committed at once.
> > 
> > Ok.  I'm attaching the complete patch.
> 
> Fine with me.

Ok, checked in.

-- 
Robert Millan

<GPLv2> I know my rights; I want my phone call!
<DRM> What use is a phone call… if you are unable to speak?
(as seen on /.)