[1/2] fdt: Factor out code to replace a phandle in place

Message ID 20180514100332.32990-2-aik@ozlabs.ru
State New
Headers show
Series
  • fdt: Support more QEMU-originated phandles
Related show

Commit Message

Alexey Kardashevskiy May 14, 2018, 10:03 a.m.
We generate a fake XICS phandle in QEMU and SLOF replaces that phandle
with the real one (i.e. SLOF's node address) in interrupt-parent and
interrupt-map properties. These properties are handled differently -
the interrupt-map is fixed in place while interrupt-parent is
decoded+encoded+set as a property.

This changes interrupt-parent fixing code to do what the interrupt-map
code does because soon we are going to have more phandles to fix and some
contain an array of phandles (such as "ibm,npu").

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 board-qemu/slof/fdt.fs | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

Comments

Thomas Huth May 18, 2018, 4:10 p.m. | #1
On 14.05.2018 12:03, Alexey Kardashevskiy wrote:
> We generate a fake XICS phandle in QEMU and SLOF replaces that phandle
> with the real one (i.e. SLOF's node address) in interrupt-parent and
> interrupt-map properties. These properties are handled differently -
> the interrupt-map is fixed in place while interrupt-parent is
> decoded+encoded+set as a property.
> 
> This changes interrupt-parent fixing code to do what the interrupt-map
> code does because soon we are going to have more phandles to fix and some
> contain an array of phandles (such as "ibm,npu").
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  board-qemu/slof/fdt.fs | 27 +++++++++++++++++++--------
>  1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
> index 8e8921f..5284ae6 100644
> --- a/board-qemu/slof/fdt.fs
> +++ b/board-qemu/slof/fdt.fs
> @@ -279,6 +279,23 @@ fdt-claim-reserve
>     2drop
>  ;
>  
> +: fdt-replace-l, 4 - swap 4 + swap ;

Why "l," ? I thought the "comma" words would be used for compiling stuff
into the dictionary, but apparently you're not doing this here?

> +: (fdt-replace-phandles) ( old new propname propnamelen node )

Please use the ( before -- after ) notation here:

( old new propname propnamelen node -- )

> +    get-property IF 2drop EXIT THEN
> +    BEGIN
> +        dup
> +    WHILE                   ( old new prop-addr prop-len )
> +        over l@
> +        4 pick = IF
> +            2 pick 2 pick l! \ replace old with new in place
> +            TRUE TO (fdt-phandle-replaced)
> +        THEN
> +        fdt-replace-l,

Maybe you could simply use the "4 - swap 4 + swap" directly here? It's
only one line of code, so not sure whether it's worth to put this into a
separate Forth word.

> +    REPEAT
> +    2drop 2drop
> +;
> +
>  \ Replace one phandle "old" with a phandle "new" in "node" and recursively
>  \ in its child nodes:
>  : fdt-replace-all-phandles ( old new node -- )
> @@ -288,14 +305,8 @@ fdt-claim-reserve
>        ( old new prop-addr prop-len  R: node )
>        fdt-replace-interrupt-map
>     THEN
> -   s" interrupt-parent" r@ get-property 0= IF
> -      ( old new prop-addr prop-len  R: node )
> -      decode-int -rot 2drop                  ( old new val  R: node )
> -      2 pick = IF                            ( old new      R: node )
> -         dup encode-int s" interrupt-parent" r@ set-property
> -         TRUE TO (fdt-phandle-replaced)
> -      THEN
> -   THEN
> +
> +   2dup s" interrupt-parent" r@ (fdt-replace-phandles)
>     \ ... add more properties that have to be fixed here ...
>     r>
>     \ Now recurse over all child nodes:       ( old new node )

Apart from the nits, this looks ok to me for small properties. But how
do you make sure that you do not accidentally replace the wrong spot in
bigger properties later?

 Thomas
Alexey Kardashevskiy May 21, 2018, 7:17 a.m. | #2
On 19/5/18 2:10 am, Thomas Huth wrote:
> On 14.05.2018 12:03, Alexey Kardashevskiy wrote:
>> We generate a fake XICS phandle in QEMU and SLOF replaces that phandle
>> with the real one (i.e. SLOF's node address) in interrupt-parent and
>> interrupt-map properties. These properties are handled differently -
>> the interrupt-map is fixed in place while interrupt-parent is
>> decoded+encoded+set as a property.
>>
>> This changes interrupt-parent fixing code to do what the interrupt-map
>> code does because soon we are going to have more phandles to fix and some
>> contain an array of phandles (such as "ibm,npu").
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  board-qemu/slof/fdt.fs | 27 +++++++++++++++++++--------
>>  1 file changed, 19 insertions(+), 8 deletions(-)
>>
>> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
>> index 8e8921f..5284ae6 100644
>> --- a/board-qemu/slof/fdt.fs
>> +++ b/board-qemu/slof/fdt.fs
>> @@ -279,6 +279,23 @@ fdt-claim-reserve
>>     2drop
>>  ;
>>  
>> +: fdt-replace-l, 4 - swap 4 + swap ;
> 
> Why "l," ? I thought the "comma" words would be used for compiling stuff
> into the dictionary, but apparently you're not doing this here?


Not just dictionary, fdt-fl.fs uses this naming approach when adding to
flatten DT. Segher suggested, looks nicer.


> 
>> +: (fdt-replace-phandles) ( old new propname propnamelen node )
> 
> Please use the ( before -- after ) notation here:
> 
> ( old new propname propnamelen node -- )
> 
>> +    get-property IF 2drop EXIT THEN
>> +    BEGIN
>> +        dup
>> +    WHILE                   ( old new prop-addr prop-len )
>> +        over l@
>> +        4 pick = IF
>> +            2 pick 2 pick l! \ replace old with new in place
>> +            TRUE TO (fdt-phandle-replaced)
>> +        THEN
>> +        fdt-replace-l,
> 
> Maybe you could simply use the "4 - swap 4 + swap" directly here? It's
> only one line of code, so not sure whether it's worth to put this into a
> separate Forth word.


"l," kind of allocates some bytes in some heap.


> 
>> +    REPEAT
>> +    2drop 2drop
>> +;
>> +
>>  \ Replace one phandle "old" with a phandle "new" in "node" and recursively
>>  \ in its child nodes:
>>  : fdt-replace-all-phandles ( old new node -- )
>> @@ -288,14 +305,8 @@ fdt-claim-reserve
>>        ( old new prop-addr prop-len  R: node )
>>        fdt-replace-interrupt-map
>>     THEN
>> -   s" interrupt-parent" r@ get-property 0= IF
>> -      ( old new prop-addr prop-len  R: node )
>> -      decode-int -rot 2drop                  ( old new val  R: node )
>> -      2 pick = IF                            ( old new      R: node )
>> -         dup encode-int s" interrupt-parent" r@ set-property
>> -         TRUE TO (fdt-phandle-replaced)
>> -      THEN
>> -   THEN
>> +
>> +   2dup s" interrupt-parent" r@ (fdt-replace-phandles)
>>     \ ... add more properties that have to be fixed here ...
>>     r>
>>     \ Now recurse over all child nodes:       ( old new node )
> 
> Apart from the nits, this looks ok to me for small properties. But how
> do you make sure that you do not accidentally replace the wrong spot in
> bigger properties later?


Unlike already present and potentially dangerous "interrupt-xxx" hacking,
the new properties I am adding are storing only phandles (one or two). It
is worth commenting though, will add.
Thomas Huth May 22, 2018, 5:23 a.m. | #3
On 21.05.2018 09:17, Alexey Kardashevskiy wrote:
> On 19/5/18 2:10 am, Thomas Huth wrote:
>> On 14.05.2018 12:03, Alexey Kardashevskiy wrote:
>>> We generate a fake XICS phandle in QEMU and SLOF replaces that phandle
>>> with the real one (i.e. SLOF's node address) in interrupt-parent and
>>> interrupt-map properties. These properties are handled differently -
>>> the interrupt-map is fixed in place while interrupt-parent is
>>> decoded+encoded+set as a property.
>>>
>>> This changes interrupt-parent fixing code to do what the interrupt-map
>>> code does because soon we are going to have more phandles to fix and some
>>> contain an array of phandles (such as "ibm,npu").
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>  board-qemu/slof/fdt.fs | 27 +++++++++++++++++++--------
>>>  1 file changed, 19 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
>>> index 8e8921f..5284ae6 100644
>>> --- a/board-qemu/slof/fdt.fs
>>> +++ b/board-qemu/slof/fdt.fs
>>> @@ -279,6 +279,23 @@ fdt-claim-reserve
>>>     2drop
>>>  ;
>>>  
>>> +: fdt-replace-l, 4 - swap 4 + swap ;
>>
>> Why "l," ? I thought the "comma" words would be used for compiling stuff
>> into the dictionary, but apparently you're not doing this here?
> 
> Not just dictionary, fdt-fl.fs uses this naming approach when adding to
> flatten DT. Segher suggested, looks nicer.

Ok, sure, but you don't *add* anything here, e.g. you also don't call
fdt-allot like in the other "fdt-something," function. IMHO the comma is
rather confusing here.

>>> +: (fdt-replace-phandles) ( old new propname propnamelen node )
>>
>> Please use the ( before -- after ) notation here:
>>
>> ( old new propname propnamelen node -- )
>>
>>> +    get-property IF 2drop EXIT THEN
>>> +    BEGIN
>>> +        dup
>>> +    WHILE                   ( old new prop-addr prop-len )
>>> +        over l@
>>> +        4 pick = IF
>>> +            2 pick 2 pick l! \ replace old with new in place
>>> +            TRUE TO (fdt-phandle-replaced)
>>> +        THEN
>>> +        fdt-replace-l,
>>
>> Maybe you could simply use the "4 - swap 4 + swap" directly here? It's
>> only one line of code, so not sure whether it's worth to put this into a
>> separate Forth word.
> 
> "l," kind of allocates some bytes in some heap.

But your "fdt-replace-l," does not allocate anything, it just increases
an address on the stack and a size on the stack at the same time.

 Thomas
Segher Boessenkool May 22, 2018, 6:04 a.m. | #4
On Tue, May 22, 2018 at 07:23:55AM +0200, Thomas Huth wrote:
> On 21.05.2018 09:17, Alexey Kardashevskiy wrote:
> > On 19/5/18 2:10 am, Thomas Huth wrote:
> >> On 14.05.2018 12:03, Alexey Kardashevskiy wrote:
> >>> +: fdt-replace-l, 4 - swap 4 + swap ;
> >>
> >> Why "l," ? I thought the "comma" words would be used for compiling stuff
> >> into the dictionary, but apparently you're not doing this here?
> > 
> > Not just dictionary, fdt-fl.fs uses this naming approach when adding to
> > flatten DT. Segher suggested, looks nicer.
> 
> Ok, sure, but you don't *add* anything here, e.g. you also don't call
> fdt-allot like in the other "fdt-something," function. IMHO the comma is
> rather confusing here.

Agreed.

> >> Maybe you could simply use the "4 - swap 4 + swap" directly here? It's
> >> only one line of code, so not sure whether it's worth to put this into a
> >> separate Forth word.
> > 
> > "l," kind of allocates some bytes in some heap.
> 
> But your "fdt-replace-l," does not allocate anything, it just increases
> an address on the stack and a size on the stack at the same time.

fdt-replace-l, is just

  /l /string

so it is much easier to understand if you just type that directly.


Segher

Patch

diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
index 8e8921f..5284ae6 100644
--- a/board-qemu/slof/fdt.fs
+++ b/board-qemu/slof/fdt.fs
@@ -279,6 +279,23 @@  fdt-claim-reserve
    2drop
 ;
 
+: fdt-replace-l, 4 - swap 4 + swap ;
+
+: (fdt-replace-phandles) ( old new propname propnamelen node )
+    get-property IF 2drop EXIT THEN
+    BEGIN
+        dup
+    WHILE                   ( old new prop-addr prop-len )
+        over l@
+        4 pick = IF
+            2 pick 2 pick l! \ replace old with new in place
+            TRUE TO (fdt-phandle-replaced)
+        THEN
+        fdt-replace-l,
+    REPEAT
+    2drop 2drop
+;
+
 \ Replace one phandle "old" with a phandle "new" in "node" and recursively
 \ in its child nodes:
 : fdt-replace-all-phandles ( old new node -- )
@@ -288,14 +305,8 @@  fdt-claim-reserve
       ( old new prop-addr prop-len  R: node )
       fdt-replace-interrupt-map
    THEN
-   s" interrupt-parent" r@ get-property 0= IF
-      ( old new prop-addr prop-len  R: node )
-      decode-int -rot 2drop                  ( old new val  R: node )
-      2 pick = IF                            ( old new      R: node )
-         dup encode-int s" interrupt-parent" r@ set-property
-         TRUE TO (fdt-phandle-replaced)
-      THEN
-   THEN
+
+   2dup s" interrupt-parent" r@ (fdt-replace-phandles)
    \ ... add more properties that have to be fixed here ...
    r>
    \ Now recurse over all child nodes:       ( old new node )