[SLOF] [PATCH slof] fdt: Pass the resulting device tree to QEMU

[Alexey Kardashevskiy]

On 02/10/17 06:34, Segher Boessenkool wrote:
> Hi Alexey,
> 
> Just some random comments, not a full review:
> 
> On Fri, Sep 29, 2017 at 07:07:29PM +1000, Alexey Kardashevskiy wrote:
>> +: fdt-struct-add ( bytes len nullterminate -- )
>> +    >r
>> +    dup >r                  ( bytes len r: nullterminate len )
>> +    fdt-struct-cur @ swap   ( bytes cur len r: nullterminate len )
>> +    move
>> +    fdt-struct-cur @        ( cur r: nullterminate len )
>> +    r> +                    ( cur r: nullterminate )
>> +    r> IF
>> +        0 over c!
>> +        1+
>> +    THEN
>> +    3 + -4 and
>> +    fdt-struct-cur !
>> +;
> 
> You want a comment on this function,

You mean text comments, not just stack comments? Ok.

> and you probably want a convenience
> word to do the zero-termination (e.g. fdt-struct-add-zero-terminated).

Seeing too many fdt-struct-cur hurts my eyes so I'd rather not.


> 
>> +: fdt-add-long ( token -- )
>> +    fdt-struct-cur @
>> +    l!
>> +    fdt-struct-cur @
>> +    4 +
>> +    fdt-struct-cur !
>> +;
> 
> : fdt-add-long ( token -- )
>    fdt-struct-cur @ l!  /l fdt-struct-cur +! ;
> 
> "add" is not such a great name, maybe you should name this like property
> encoding (and maybe structure it like it even), so fdt-encode-int etc.?
> 
>> +: skip-to-next ( cur -- cur )
>> +    BEGIN
>> +        dup c@
>> +    WHILE
>> +        1+
>> +    REPEAT
>> +    4 + -4 and
>> +;
> 
> : skip-to-next ( addr -- addr )  dup zcount + 1+ 4 #aligned ;
> 
> Skip to next what?  "fdt-skip-string" perhaps?
> 
>> +: string-equal ( name namelen str -- true | false )
>> +    3dup swap comp 0 <> IF
>> +        3drop
>> +        false
>> +        EXIT
>> +    THEN
>> +    + c@ 0 = nip
>> +
>> +    dup IF
>> +        fdt-strings-reused @ 1+ fdt-strings-reused !
>> +    THEN
>> +;
> 
> So "str" is a zero-terminated string here.
> 
> : zstring=  ( str len zstr -- flag )
>    2dup + c@ IF 3drop false EXIT THEN  swap comp 0= ;

Thanks, this is better :)

But - is it really necessary to make words a single line? What is exactly
that you win when you do this? It is really hard to read.


> \ Not a great name, it does this "reused" thing, too?
> : string-equal ( name namelen str -- true | false )
>    zstring= dup IF 1 fdt-strings-reused +! ;


My bad, "reused" thing is a leftover from debug code which tells how many
strings this has saved, you can ignore it.


>> +: fdt-get-string ( name namelen -- nameoff )
>> +    \ lookup
>> +    fdt-strings @
>> +    BEGIN
>> +        dup fdt-strings-cur @
>> +        = not
> 
> Don't use NOT, it does not do what you think (it is INVERT, but you want
> 0= here; could just say <> of course).

Yup, a bug, thanks.

> 
> This word is so big that it is very hard to follow, btw.

Hmmm. I'll split out the lookup bit. fdt-flatten-tree is even bigger but
you did not make the same comment about it.

>
>> +: fdt-begin-node ( name namelen -- )
>> +    OF_DT_BEGIN_NODE fdt-add-long
>> +    2dup 1 = swap c@ 2F = and  \ is it "/"?
> 
> This would be ideal for COMPARE except that was never implemented ;-)
>    s" /" compare
> (It's also probably more readable to write   [char] /   instead of 2F,
> if you don't want to use the string words).

Sure, it has just been a while since I touched forth :)


> 
> I think this takes so long on huge device trees because you do way too
> many string compares, but it is hard to tell.

I'll double check but 2.8sec on such a massive guest is really not much as
the rest takes lot lot more time.

Thanks for the comment. Are you still going to do the full review or I can
respin? :)


One question while I remember: firstly I looked at how "(.properties)" and
".property" work and these things do not use "next-property", it is
node>properties, link>, etc - is this described somewhere (1275?) or it is
the SLOF implementation detail? It is probably faster but definitely harder
to read.




-- 
Alexey