[commit] r733 - trunk/openbios-devel/libopenbios

[repository service]

Author: mcayland
Date: Sat Apr  3 15:03:39 2010
New Revision: 733
URL: http://tracker.coreboot.org/trac/openbios/changeset/733

Log:
Another couple of client interface fixes:

i) Ensure that the CIF caller's idea of pb->nret is always respected, but if there is a mismatch then log a warning if DEBUG_CIF 
is enabled, then correct the stack and continue.

ii) Correct the code path for call-method and interpret to ensure they ignore the extra status variable pushed onto the stack by 
client-call-iface.

Signed-off-by: Mark Cave-Ayland

Modified:
   trunk/openbios-devel/libopenbios/client.c

Modified: trunk/openbios-devel/libopenbios/client.c
==============================================================================
--- trunk/openbios-devel/libopenbios/client.c	Sat Apr  3 10:25:47 2010	(r732)
+++ trunk/openbios-devel/libopenbios/client.c	Sat Apr  3 15:03:39 2010	(r733)
@@ -245,7 +245,11 @@
    push_str( pb->service );
    fword("client-call-iface");

-	for( i=0; i<pb->nret && dstackcnt > dstacksave; i++ ) {
+	/* Drop the return code from client-call-iface (status is handled by the 
+	   catch result which is the first parameter below) */
+	POP();
+
+	for( i=0; i<pb->nret; i++ ) {
                 val = POP();
    	pb->args[pb->nargs + i] = val;

@@ -253,9 +257,10 @@
    	if( !i && val )
    		break;
    }
+
 #ifdef DEBUG_CIF
    /* useful for debug but not necessarily an error */
-	if( i != pb->nret || dstacksave != dstacksave ) {
+	if( i != pb->nret || dstackcnt != dstacksave ) {
                 printk("%s '%s': possible argument error (%ld--%ld) got %d\n",
    	       pb->service, (char*)pb->args[0], pb->nargs-2, pb->nret, i );
    }
@@ -266,6 +271,7 @@
         }
         printk("\n");
 #endif
+
    dstackcnt = dstacksave;
    return 0;
 }
@@ -279,6 +285,7 @@
    if( pb->nargs < 0 || pb->nret < 0 ||
             pb->nargs + pb->nret > PROM_MAX_ARGS)
    	return -1;
+
 #ifdef DEBUG_CIF
    dump_service(pb);
 #endif
@@ -302,16 +309,21 @@
    	return -1;
    }

-	for( pb->nret=0; dstackcnt > dstacksave ; pb->nret++ )
-		pb->args[pb->nargs + pb->nret] = POP();
+	for( i=0; i<pb->nret ; i++ )
+		pb->args[pb->nargs + i] = POP();

-#ifdef DEBUG_CIF
    if( dstackcnt != dstacksave ) {
-		printk("service %s: argument count error (%d %d)\n",
+#ifdef DEBUG_CIF
+		printk("service %s: possible argument error (%d %d)\n",
    	       pb->service, i, dstackcnt - dstacksave );
+#endif
+		/* Some clients request less parameters than the CIF method
+		returns, e.g. getprop with OpenSolaris. Hence we drop any
+		stack parameters after issuing a warning above */
    	dstackcnt = dstacksave;
    }

+#ifdef DEBUG_CIF
    dump_return(pb);
 #endif
    return 0;

[Segher Boessenkool]

> /* Some clients request less parameters than the CIF method
> returns, e.g. getprop with OpenSolaris.

Really?  getprop has only one return value, and it is essential to  
check it!
Well, maybe they always call getproplen before.  But it's always an  
error
to have the number of return values (or arguments) not match with  
what is
required.

Segher

[Tarl Neustaedter]

On 2010-4-3 11:21 AM, Segher Boessenkool wrote:
> > /* Some clients request less parameters than the CIF method
> > returns, e.g. getprop with OpenSolaris.
>
> Really? getprop has only one return value, and it is essential to 
> check it!
> Well, maybe they always call getproplen before. [...]

I have a vague recollection that for historical reasons that's exactly 
what Solaris does. Something about non-terminated null strings if the 
property was too large for the buffer (the size returned in getprop is 
the min of buffer size and property size). Buried in the mists of 
antiquity, back in the days when it wasn't unusual to have the kernel 
fall over doing a printf() of a too-long string.
> But it's always an error to have the number of return values (or 
> arguments) not match with what is required.

Oh, it's certainly the wrong thing to do, and when writing new code you 
should avoid it. But evidently there is existing code which does it. 
Since the interface allows specifying fewer requested return values, we 
have to handle the case where they aren't asked for.

I should note that there is some commentary in section 6.3.1 of 1275 
which explains the additional return value pushed by client-call-iface:

/In addition to the ret1 through retN values that are returned in the 
array, the client interface handler returns a single value as specified 
in the Open Firmware supplement for the appropriate ISA (see 2.1). That 
value indicates whether the transfer of control to the Open Firmware 
succeeded or failed. If the requested service is unavailable or if the 
control transfer failed, the client interface handler returns the value 
–1; otherwise, it returns the value zero./

[Mark Cave-Ayland]

Segher Boessenkool wrote:
> Really?  getprop has only one return value, and it is essential to check 
> it!
> Well, maybe they always call getproplen before.

Yeah, that's what I see happening during my OpenSolaris boot here. So at 
the least the CIF client can get the length information it needs.
> But it's always an error
> to have the number of return values (or arguments) not match with what is
> required.

It doesn't seem too clever. The way I've interpreted the number of 
returned parameters field in the CIF parameter block is that it's the 
maximum number of arguments the client would like returned. The original 
spec isn't clear whether the number of returned items is specified by 
the caller or set by the BIOS CIF implementation on return. But at least 
we can correct for it by checking the stack depth. Maybe Tarl has some 
more insight on this?

ATB,

Mark.

[Tarl Neustaedter]

On 2010-4-3 12:02 PM, Mark Cave-Ayland wrote:
> [...]
> It doesn't seem too clever. The way I've interpreted the number of 
> returned parameters field in the CIF parameter block is that it's the 
> maximum number of arguments the client would like returned. The 
> original spec isn't clear whether the number of returned items is 
> specified by the caller or set by the BIOS CIF implementation on 
> return. But at least we can correct for it by checking the stack 
> depth. Maybe Tarl has some more insight on this?

My interpretation of the spec is that N-args and N-returns *define* the 
number of arguments taken and returned. If the called method generates a 
different number of returns than the caller asked for, the interface 
must either generate or discard the additional values.

In section 6.3.2.6 of 1275, for the general case "interpret" call, we see:

/N-args and N-returns are stored in the argument array and may be 
different for different calls to interpret. If the number of items X 
left on the Forth data stack as a result of the execution of cmd is less 
than N-returns, only stack-result1, ..., stack-resultX are modified; 
other elements of the returned values portion of the argument array are 
unaffected. If X is more than N-returns, additional items are popped 
from the Forth data stack after setting stack-result1, ..., 
stack-resultQ so that, in all cases, the execution of interpret results 
in no net change to the depth of the Forth data stack./

This is specific to the "interpret" call (and also in "call-method"), 
but it suggests the approach that the caller is not required to 
precisely match the called routine.

[Mark Cave-Ayland]

Tarl Neustaedter wrote:
> My interpretation of the spec is that N-args and N-returns *define* the 
> number of arguments taken and returned. If the called method generates a 
> different number of returns than the caller asked for, the interface 
> must either generate or discard the additional values.

Hi Tarl,

Thanks for the clarification on this. This was the conclusion I reached 
but from a slightly different perspective: i.e. it is the CIF client 
that passes the parameter block into the CIF interface. Hence if the 
client specifies 1 return argument then it is likely that only enough 
memory has been allocated for 1 argument, and so anything else should 
just be ignored to prevent memory buffer overruns.

ATB,

Mark.