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

[Segher Boessenkool]

Hi!

On Mon, Oct 02, 2017 at 12:08:32PM +1100, Alexey Kardashevskiy wrote:
> On 02/10/17 06:34, Segher Boessenkool wrote:
> > 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.

Yeah, a comment on what the word does, since the name doesn't make it
obvious.  Better is to have a better name, but I can't think of any
either.  Maybe this needs to factored a bit better?

> > 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.

Yeah.

So maybe something like

VALUE fdt-here  \ instead of fdt-struct-cur
: fdt-allot  fdt-here + to fdt-here ;
: fdt-c,  fdt-here  1 fdt-allot  c! ;
: fdt-align  fdt-here 4 #aligned fdt-here - ; \ or fill with zeroes?
: fdt-str, ( str len -- )  fdt-here over fdt-allot  swap move ;
: fdt-ztr, ( str len -- )  fdt-str,  0 fdt-c, ;
\ and now use   fdt-str, fdt-align   instead of   false fdt-struct-add
\ and just  fdt-ztr, fdt-align   for   true fdt-struct-add

> >> +: 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 +! ;

This would be   fdt-l,   then.  Using similar names to the standard data
space allocation words makes things easy to understand (and these are a
known good design so you don't have to spend time designing a good
interface :-) )

> > 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.

You can use whatever style you like best :-)  I find this more readable,
or perhaps

: zstring=  ( str len zstr -- flag )
   2dup + c@ IF
     3drop false EXIT THEN
   swap comp 0= ;

if you want to be fancy.  It's a good idea to keep your words short and
sweet, and then indent etc. doesn't matter anyway.

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

Ah :-)

> > 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.

But fdt-flatten-tree is mostly a list of things to do in order.

I didn't comment on it because I haven't reviewed the whole thing, btw.

> > 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.

Ah, right.  You have to create that tree in the first place already :-)

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

Sure, but it will be a while.

> 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.

Implementation details.

node>properties is defined in node.fs, it just gets the address of a
struct field.

The properties are not a linked list, but a Forth wordlist.  This is a)
faster, because wordlists are optimised a bit, say ten times faster; and
b) this allows use to run any code we want to get a property, not just
lookups of static data.  From property.fs:

\ Words on the property list for a node are actually executable words,
\ that return the address and length of the property's data.  Special
\ nodes like /options can have their properties use specialized code to
\ dynamically generate their data; most nodes just use a 2CONSTANT.


Segher