[OpenBIOS] "next-property" reloaded !

[Mark Cave-Ayland]

On 18/02/14 01:08, Olivier Danet wrote:

> Hello !
> I have tried to recreate the 3 known variants of next-property:
>
> Standard : a) true "string" b) false c) false
> Sun Ultra10 : a) -1 "string" b) nothing c) nothing
> PowerPC iBook : a) -1 "string" b) -1 0 0 c) 0
>
> Here is a tentative patch :
>
> Index: forth/admin/devices.fs
> ===================================================================
> --- forth/admin/devices.fs (révision 1269)
> +++ forth/admin/devices.fs (copie de travail)
> @@ -451,7 +451,7 @@
> ?active-package dup >r if
> 0 0
> begin
> - r@ next-property
> + r@ next-property-std
> while
> cr 2dup dup -rot type
> begin ." " 1+ dup d# 26 >= until drop
> Index: forth/device/property.fs
> ===================================================================
> --- forth/device/property.fs (révision 1269)
> +++ forth/device/property.fs (copie de travail)
> @@ -63,14 +63,14 @@
> ;
>
> \ From package (5.3.4.1)
> -: next-property
> +: next-property-std
> ( previous-str previous-len phandle -- false | name-str name-len true )
>  >r
> 2dup 0= swap 0= or if
> 2drop r> >dn.properties @
> else
> r> find-property dup if @ then
> - ?dup if >prop.next @ then
> + dup if >prop.next @ then
> then
>
> ?dup if
> @@ -81,7 +81,57 @@
> then
> ;
>
> +: next-property-ppc
> +( previous-str previous-len phandle -- false | name-str name-len true |
> 0 0 true )
> + >r
> + 2dup 0= swap 0= or if
> + 2drop r> >dn.properties @ true
> + else
> + r> find-property dup if @ then
> + dup if >prop.next @ true then
> + then
>
> + if
> + dup if
> + >prop.name @ dup cstrlen
> + ( phandle name-str name-len )
> + else
> + 0
> + ( 0 0 )
> + then
> + true
> + else
> + false
> + then
> +;
> +
> +: next-property-std-bis
> + next-property-ppc
> + dup
> + if over 0=
> + if
> + 2drop
> + then
> + then
> +;
> +
> +: next-property-s64
> + next-property-std
> + ?dup drop
> +;
> +
> +: next-property
> + [IFDEF] CONFIG_PPC
> + next-property-ppc
> + [ELSE]
> + [IFDEF] CONFIG_SPARC64
> + next-property-s64
> + [ELSE]
> + next-property-std
> + [THEN]
> + [THEN]
> +;
> +
> \
> \ 5.3.5.4 Property value access
> \
> Index: forth/system/ciface.fs
> ===================================================================
> --- forth/system/ciface.fs (révision 1269)
> +++ forth/system/ciface.fs (copie de travail)
> @@ -121,7 +121,7 @@
>
> ( buf prev prev_len )
>
> - r> next-property if
> + r> next-property-std if
> ( buf name name_len )
> dup 1+ -rot ci-strcpy drop 1
> else
> ===================================================================
>
> there are :
> next-property-std = standard, used by Sparc32 "romvec" C code and by
> forth code, like ".properties"
> next-property-s64 = Sparc64 version, built from next-property-std
> next-property-ppc = PowerPC version
> next-property-std-bis = Alternate standard version, built from the PPC
> one (e.g. when compiling for PPC target)
>
> Tests :
>
> showstack
> cd screen
>
> \ Sparc32
> " " ?active-package next-property-std cr . type cr
> " name" ?active-package next-property-std cr . type cr
> " interrupts" ?active-package next-property-std cr . cr
> " ttttt" ?active-package next-property-std cr . cr
>
> \ PowerPC
> " " ?active-package next-property-ppc cr . type cr
> " name" ?active-package next-property-ppc cr . type cr
> " interrupts" ?active-package next-property-ppc cr . . . cr
> " ttttt" ?active-package next-property-ppc cr . cr
>
> \ Sparc64
> " " ?active-package next-property-s64 cr . type cr
> " name" ?active-package next-property-s64 cr . type cr
> " interrupts" ?active-package next-property-s64 cr
> " ttttt" ?active-package next-property-s64 cr
>
>
> " " ?active-package next-property-std-bis cr . type cr
> " name" ?active-package next-property-std-bis cr . type cr
> " interrupts" ?active-package next-property-std-bis cr . cr
> " ttttt" ?active-package next-property-std-bis cr . cr
>
> Currently all variants are available simultaneously.
> Additional [IFDEF] can be used to hide some variants according
> to compilation options, generate the -std from the -ppc,..
>
> One more oddity:
> NetBSD is not the only one to do strange things with properties.
> In OpenBSD, one can find :
> arch/sparc/dev/sbus.c:int sbus_testdma(sc, ca)
> ...
> getpropint(0, "slave-only", 0)
> ...
> arch/sparc/dev/sbus.c:int sbus_print(args, sbus)
> ...
> getpropint(0, sl, 0)
> ...
> Here, 0 should select the 'root' node.
> It really looks like a bug in OpenBSD.
> It does not prevent the OS from running though.
>
> Olivier

Hi Olivier,

Thanks for doing this patch, and apologies for the delay in getting back 
to you as it has given me quite a few things to think about!

My current thoughts at the moment are that the reason these particular 
differences in next-property have only just been discovered is because 
it is extremely unlikely that anyone would call it directly. So as we 
discovered from Zoltan, PPC/SPARC64 would call next-property via the 
client interface, and we know that SPARC32 would call next-property via 
the romvec block.

I guess the only way these bugs could be encountered would be if someone 
evaluated a Forth string containing next-property directly, and why 
would they bother if OpenBIOS already provides a method to do this via 
CIF/romvec?

Therefore what I would suggest is that we hold on this patch for the 
moment and instead could you provide an alternative that fixes the 
return from next-property in obp_nextprop instead? That way we have the 
results of your research preserved in the archives in case we need to 
rethink this later, plus the patch itself is much simpler making it much 
less likely to affect other platforms.


ATB,

Mark.