[commit] r915 - trunk/openbios-devel/arch/sparc32

[repository service]

Author: mcayland
Date: Mon Oct 18 21:32:16 2010
New Revision: 915
URL: http://tracker.coreboot.org/trac/openbios/changeset/915

Log:
Fix the placing of Forth arguments on the stack when calling obp_fortheval_v2() via romvec on SPARC32.

The extra stack arguments are actually placed within %o1-%o5 but unfortunately there doesn't seem to be a way of passing the 
number of parameters using the romvec API. Hence we go through the argument list and start pushing arguments onto the Forth 
stack from the first non-zero argument before executing the Forth string.

Signed-off-by: Mark Cave-Ayland

Modified:
   trunk/openbios-devel/arch/sparc32/openprom.h
   trunk/openbios-devel/arch/sparc32/romvec.c

Modified: trunk/openbios-devel/arch/sparc32/openprom.h
==============================================================================
--- trunk/openbios-devel/arch/sparc32/openprom.h	Sun Oct 17 14:15:44 2010	(r914)
+++ trunk/openbios-devel/arch/sparc32/openprom.h	Mon Oct 18 21:32:16 2010	(r915)
@@ -124,7 +124,7 @@
    /* Evaluate a forth string, not different proto for V0 and V2->up. */
    union {
    	void (*v0_eval)(int len, char *str);
-		void (*v2_eval)(char *str);
+		void (*v2_eval)(char *str, int arg0, int arg1, int arg2, int arg3, int arg4);
    } pv_fortheval;

const struct linux_arguments_v0 * const *pv_v0bootargs;

Modified: trunk/openbios-devel/arch/sparc32/romvec.c
==============================================================================
--- trunk/openbios-devel/arch/sparc32/romvec.c	Sun Oct 17 14:15:44 2010	(r914)
+++ trunk/openbios-devel/arch/sparc32/romvec.c	Mon Oct 18 21:32:16 2010	(r915)
@@ -412,15 +412,39 @@
     return 0;
 }

-static void obp_fortheval_v2(char *str)
+static void obp_fortheval_v2(char *str, int arg0, int arg1, int arg2, int arg3, int arg4)
 {
-  // for now, move something to the stack so we
-  // don't get a stack underrun.
-  //
-  // FIXME: find out why solaris doesnt put its stuff on the stack
-  //
-  fword("0");
-  fword("0");
+  int pusharg = 0;
+
+  // It seems Solaris passes up to 5 arguments which should be pushed onto the Forth
+  // stack for execution. However the API doesn't provide for a way to pass the number
+  // of arguments actually passed. Hence we start at arg4 and then starting from the
+  // first non-zero argument, we push all subsequent arguments onto the stack down to
+  // arg0.
+
+  if (arg4) {
+    PUSH(arg4);
+    pusharg = 1;
+  }
+
+  if (arg3 || pusharg == 1 ) {
+    PUSH(arg3);
+    pusharg = 1;
+  }
+
+  if (arg2 || pusharg == 1) {
+    PUSH(arg2);
+    pusharg = 1;
+  }
+
+  if (arg1 || pusharg == 1 ) {
+    PUSH(arg1);
+    pusharg = 1;
+  }
+
+  if (arg0 || pusharg == 1) {
+    PUSH(arg0);
+  }

DPRINTF("obp_fortheval_v2(%s)\n", str);
   push_str(str);

[Artyom Tarasenko]

On Mon, Oct 18, 2010 at 9:32 PM, repository service  wrote:
> Author: mcayland
> Date: Mon Oct 18 21:32:16 2010
> New Revision: 915
> URL: http://tracker.coreboot.org/trac/openbios/changeset/915
>
> Log:
> Fix the placing of Forth arguments on the stack when calling obp_fortheval_v2() via romvec on SPARC32.
>
> The extra stack arguments are actually placed within %o1-%o5 but unfortunately there doesn't seem to be a way of passing the
> number of parameters using the romvec API. Hence we go through the argument list and start pushing arguments onto the Forth
> stack from the first non-zero argument before executing the Forth string.
>
> Signed-off-by: Mark Cave-Ayland
>
> Modified:
>   trunk/openbios-devel/arch/sparc32/openprom.h
>   trunk/openbios-devel/arch/sparc32/romvec.c
> Modified: trunk/openbios-devel/arch/sparc32/openprom.h
>
> --- trunk/openbios-devel/arch/sparc32/openprom.h        Sun Oct 17 14:15:44 2010        (r914)
> +++ trunk/openbios-devel/arch/sparc32/openprom.h        Mon Oct 18 21:32:16 2010        (r915)
> @@ -124,7 +124,7 @@
>        /* Evaluate a forth string, not different proto for V0 and V2->up. */
>        union {
>                void (*v0_eval)(int len, char *str);
> void (*v2_eval)(char *str);
> void (*v2_eval)(char *str, int arg0, int arg1, int arg2, int arg3, int arg4);
>
> } pv_fortheval;
>
> const struct linux_arguments_v0 * const *pv_v0bootargs;
> Modified: trunk/openbios-devel/arch/sparc32/romvec.c
>
> --- trunk/openbios-devel/arch/sparc32/romvec.c  Sun Oct 17 14:15:44 2010        (r914)
> +++ trunk/openbios-devel/arch/sparc32/romvec.c  Mon Oct 18 21:32:16 2010        (r915)
> @@ -412,15 +412,39 @@
>     return 0;
>  }
>
> -static void obp_fortheval_v2(char *str)
> +static void obp_fortheval_v2(char *str, int arg0, int arg1, int arg2, int arg3, int arg4)
>  {
> // for now, move something to the stack so we
> // don't get a stack underrun.
> //
> // FIXME: find out why solaris doesnt put its stuff on the stack
> //
> fword("0");
> fword("0");
> int pusharg = 0;
> // It seems Solaris passes up to 5 arguments which should be pushed onto the Forth
> // stack for execution. However the API doesn't provide for a way to pass the number
> // of arguments actually passed. Hence we start at arg4 and then starting from the
> // first non-zero argument, we push all subsequent arguments onto the stack down to
> // arg0.

Why do you think it's up to 5, and not just always 5?
> if (arg4) {
> PUSH(arg4);
> pusharg = 1;
> }
> if (arg3 || pusharg == 1 ) {
> PUSH(arg3);
> pusharg = 1;
> }
> if (arg2 || pusharg == 1) {
> PUSH(arg2);
> pusharg = 1;
> }
> if (arg1 || pusharg == 1 ) {
> PUSH(arg1);
> pusharg = 1;
> }
> if (arg0 || pusharg == 1) {
> PUSH(arg0);
> }
>
> DPRINTF("obp_fortheval_v2(%s)\n", str);
>   push_str(str);

[Mark Cave-Ayland]

Artyom Tarasenko wrote:
> Why do you think it's up to 5, and not just always 5?

Mainly by looking at the Forth; it seems that all the code snippets I've 
seen expect the stack to be preserved. Otherwise if we always push all 5 
arguments, we'll end up with an excess of 0s being built-up on the stack 
by every call.

ATB,

Mark.

[Artyom Tarasenko]

On Tue, Oct 19, 2010 at 11:57 AM, Mark Cave-Ayland
 wrote:
> Artyom Tarasenko wrote:
> > Why do you think it's up to 5, and not just always 5?
>
> Mainly by looking at the Forth; it seems that all the code snippets I've
> seen expect the stack to be preserved. Otherwise if we always push all 5
> arguments, we'll end up with an excess of 0s being built-up on the stack by
> every call.

I agree that the stack should be preserved, but why not just do it
literary by throwing away unused arguments?
In the current implementation there is no way to pass 0 as the last
argument. Do you think it's ok?
> ATB,
>
> Mark.
>
> Sirius Labs: http://www.siriusit.co.uk/labs

[Mark Cave-Ayland]

Artyom Tarasenko wrote:
> > Mainly by looking at the Forth; it seems that all the code snippets I've
> > seen expect the stack to be preserved. Otherwise if we always push all 5
> > arguments, we'll end up with an excess of 0s being built-up on the stack by
> > every call.
>
> I agree that the stack should be preserved, but why not just do it
> literary by throwing away unused arguments?
> In the current implementation there is no way to pass 0 as the last
> argument. Do you think it's ok?

I think so - it's difficult to know. That was the other option of 
course, which is to push everything onto the stack and then clear the 
stack after the call has finished. But this comes down to does the 
client program expect the stack to be preserved across subsequent calls 
to obp_fortheval_v2? The current approach does this, although we don't 
know whether it's a specific requirement or not.

ATB,

Mark.

[Mark Cave-Ayland]

Artyom Tarasenko wrote:
> I agree that the stack should be preserved, but why not just do it
> literary by throwing away unused arguments?
> In the current implementation there is no way to pass 0 as the last
> argument. Do you think it's ok?

Thinking about this more, we actually have a few more tools in our 
arsenal working at the C level ;)

What do you think of the following patch? It should solve your problem 
with the last 0 argument, preserves stack content between calls and is 
probably easier to read too.

ATB,

Mark.

[Artyom Tarasenko]

On Tue, Oct 19, 2010 at 2:44 PM, Mark Cave-Ayland
 wrote:
> Artyom Tarasenko wrote:
> > I agree that the stack should be preserved, but why not just do it
> > literary by throwing away unused arguments?
> > In the current implementation there is no way to pass 0 as the last
> > argument. Do you think it's ok?
>
> Thinking about this more, we actually have a few more tools in our arsenal
> working at the C level ;)
>
> What do you think of the following patch? It should solve your problem with
> the last 0 argument, preserves stack content between calls and is probably
> easier to read too.

Yes, I like this version more.

[Mark Cave-Ayland]

Artyom Tarasenko wrote:
> > What do you think of the following patch? It should solve your problem with
> > the last 0 argument, preserves stack content between calls and is probably
> > easier to read too.
>
> Yes, I like this version more.

Okay cool. I think we can safely guarantee that any arguments left on 
the stack by the call itself are destroyed, since all of the Forth 
examples I've seen use the *! operators to write the results to memory 
before exiting. Does anyone else have a preference one way or the other 
on this patch?

Also have you had a chance to look at my other email related to 
IMAGE_STACK_SIZE and context switching?

ATB,

Mark.

[Andreas Färber]

Am 19.10.2010 um 16:22 schrieb Mark Cave-Ayland:
> Artyom Tarasenko wrote:
> > > What do you think of the following patch? It should solve your  
> > > problem with
> > > the last 0 argument, preserves stack content between calls and is  
> > > probably
> > > easier to read too.
> >
> > Yes, I like this version more.
>
> [...] Does anyone else have a preference one way or the other on  
> this patch?

I concur with Artyom. The client interface does a similar stack check/ 
correction iirc.
Often simple is better, and your new patch indeed looks much more  
readable to me.

Haven't tested either one yet; which version of Solaris did you use?

Andreas

[Mark Cave-Ayland]

Andreas Färber wrote:
> > [...] Does anyone else have a preference one way or the other on this 
> > patch?
>
> I concur with Artyom. The client interface does a similar stack 
> check/correction iirc.
> Often simple is better, and your new patch indeed looks much more 
> readable to me.

Okay - thanks :)
> Haven't tested either one yet; which version of Solaris did you use?

It was a Solaris 8 installation CD ISO. Though as noted before, you need 
to bump IMAGE_STACK_SIZE to get further into the boot due to memory 
corruption issues.

ATB,

Mark.