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

Message ID 20180514100332.32990-2-aik@ozlabs.ru
State Superseded
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
Alexey Kardashevskiy May 29, 2018, 7:59 a.m. | #5
On 22/5/18 4:04 pm, Segher Boessenkool wrote:
> 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.

Ok, I got it wrong there.


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

Is it? It is not a string I am advancing on and using something+string on
that is misleading imho. I'll do simple "4 - swap 4 + swap".
Alexey Kardashevskiy May 29, 2018, 8:06 a.m. | #6
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").

[snip]

>> +
>>  \ 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?

btw when I tried updating the commit log, I realized that I do not
understand why we fix the "interrupt-parent" at all. Git points to
https://github.com/aik/SLOF/commit/82954d but that does not explain it
either, do you have any memories about that? :)
Thomas Huth May 29, 2018, 8:43 a.m. | #7
On 29.05.2018 10:06, Alexey Kardashevskiy wrote:
[...]
> btw when I tried updating the commit log, I realized that I do not
> understand why we fix the "interrupt-parent" at all. Git points to
> https://github.com/aik/SLOF/commit/82954d but that does not explain it
> either, do you have any memories about that? :)

"interrupt-parent" is a standard property defined in chapter 5 here:
https://www.openbios.org/data/docs/rec.intmap.d09.pdf
Looks like we're currently not using it in QEMU, but since it's a
standard property, I guess I implemented support for this in SLOF
anyway, just to be sure.

 Thomas
Segher Boessenkool May 30, 2018, 2:13 a.m. | #8
On Tue, May 29, 2018 at 05:59:14PM +1000, Alexey Kardashevskiy wrote:
> On 22/5/18 4:04 pm, Segher Boessenkool wrote:
> >>>> 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.
> 
> Is it? It is not a string I am advancing on and using something+string on
> that is misleading imho. I'll do simple "4 - swap 4 + swap".

"string" just means "memory region": an address and a length.  "/string"
shortens a memory region at the front (doing it at the end is just "-").


Segher
David Gibson June 5, 2018, 5:19 a.m. | #9
On Mon, May 14, 2018 at 08:03:31PM +1000, 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>

Hmmmmmmmm.....

Every time I see more of these contortions to deal with the mismatch
between SLOF assigned and qemu assigned phandles, I wonder anew if the
right solution isn't just for SLOF to keep the qemu assigned ones, and
only assign its own for ones that don't have them in the first place.

I mean, yes, it's very non-traditional from an OF point of view, but I
really have a hard time seeing it being nastier than this growing
collection of ad-hoc hacks.

> ---
>  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 ;
> +
> +: (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 )
Alexey Kardashevskiy June 7, 2018, 5:30 a.m. | #10
On 5/6/18 3:19 pm, David Gibson wrote:
> On Mon, May 14, 2018 at 08:03:31PM +1000, 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>
> 
> Hmmmmmmmm.....
> 
> Every time I see more of these contortions to deal with the mismatch
> between SLOF assigned and qemu assigned phandles, I wonder anew if the
> right solution isn't just for SLOF to keep the qemu assigned ones, and
> only assign its own for ones that don't have them in the first place.
> 
> I mean, yes, it's very non-traditional from an OF point of view, but I
> really have a hard time seeing it being nastier than this growing
> collection of ad-hoc hacks.


I made these patches to add nvlink2 pass through support which means I have
to have cross references between PCI device nodes in the device tree.

It turns out that if I simply put unique numbers to "phandle" properties in
QEMU and not patch them, they remain unchanged and then the NVIDIA driver
can actually find nodes by these QEMU-made phandles. I cannot find places
in the (guest) kernel which would rely on phandles being physical addresses
- the client interface is used from prom_init.c and that is it.

In general, we have 2 types of phandles - "phandle" and "linux,phandle". If
"phandle" is present at the prom_init stage, we keep that, if it is not,
then prom_init creates "linux,phandle" so we end up having one of them but
not both. We could probably change QEMU to store its phandles as
"linux,phandle" and simply not touch "phandle" properties. Or use
"ibm,phandle".

But the comment at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/fdt.c?h=v4.16#n221
suggests these are not interchangeable and better be equal. Even though
"ibm,phandle" values can differ from "phandle" ones. Hm.

The comment came from there :)
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=04b954a673dd0


> 
>> ---
>>  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 ;
>> +
>> +: (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 )
>
David Gibson June 12, 2018, 2:58 a.m. | #11
On Thu, Jun 07, 2018 at 03:30:20PM +1000, Alexey Kardashevskiy wrote:
> On 5/6/18 3:19 pm, David Gibson wrote:
> > On Mon, May 14, 2018 at 08:03:31PM +1000, 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>
> > 
> > Hmmmmmmmm.....
> > 
> > Every time I see more of these contortions to deal with the mismatch
> > between SLOF assigned and qemu assigned phandles, I wonder anew if the
> > right solution isn't just for SLOF to keep the qemu assigned ones, and
> > only assign its own for ones that don't have them in the first place.
> > 
> > I mean, yes, it's very non-traditional from an OF point of view, but I
> > really have a hard time seeing it being nastier than this growing
> > collection of ad-hoc hacks.
> 
> 
> I made these patches to add nvlink2 pass through support which means I have
> to have cross references between PCI device nodes in the device tree.
> 
> It turns out that if I simply put unique numbers to "phandle" properties in
> QEMU and not patch them, they remain unchanged and then the NVIDIA driver
> can actually find nodes by these QEMU-made phandles. I cannot find places
> in the (guest) kernel which would rely on phandles being physical addresses
> - the client interface is used from prom_init.c and that is it.

No, the phandles definitely should be opaque to the guest.  They might
not be to SLOF itself, though.

> In general, we have 2 types of phandles - "phandle" and
> "linux,phandle".

Those aren't two types, just an older and newer name for the same thing.

> If
> "phandle" is present at the prom_init stage, we keep that, if it is not,
> then prom_init creates "linux,phandle" so we end up having one of them but
> not both. We could probably change QEMU to store its phandles as
> "linux,phandle" and simply not touch "phandle" properties. Or use
> "ibm,phandle".
> 
> But the comment at
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/fdt.c?h=v4.16#n221
> suggests these are not interchangeable and better be equal.

Rather they *are* interchangeable.  If you have both they absolutely
must be equal.

> Even though
> "ibm,phandle" values can differ from "phandle" ones. Hm.

I have no idea what "ibm,phandle" is about or where they come from.

> 
> The comment came from there :)
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=04b954a673dd0
> 
> 
> > 
> >> ---
> >>  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 ;
> >> +
> >> +: (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 )
> > 
> 
>

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 )