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

[David Gibson]

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 at 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 )
> > 
> 
> 




-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/slof/attachments/20180612/fee8e087/attachment.sig>