[OpenBIOS] "next-property" reloaded !

[Olivier Danet]

On 03/03/2014 22:12, Mark Cave-Ayland wrote:
> 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.

Just push the one-liner Forth patch I submitted two months ago...

Regards.
Olivier