Re: Virtualization difficulty -- phandles

[Frank Rowand <frowand.list-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

On 07/25/17 00:50, David Gibson wrote:
> On Mon, Jul 24, 2017 at 10:09:48AM -0700, Frank Rowand wrote:
>> Hi David,
>>
>> (Adding Pantelis and Tom, since I'm going somewhat off-topic from
>> the original thread, and they are impacted by what I am asking.)
>>
>> On 07/15/17 22:35, David Gibson wrote:
>>> On Thu, Jul 13, 2017 at 09:47:01AM -0700, Florian Fainelli wrote:
>>>> On 07/12/2017 09:23 PM, Cyril Novikov wrote:
>>>>> On 7/12/2017 10:10 AM, Florian Fainelli wrote:
>>>>>> On 07/11/2017 11:15 PM, Cyril Novikov wrote:
>>>>>>> Hi, all!
>>>>>>>
>>
>> < snip >
>>
>>>   The
>>> phandle fixup information goes into the special __local_fixups__ and
>>> __fixups__ nodes (which have gratuitiously different format, but
>>> that's a rant for elsewhere).
>>
>> < snip >
>>
>> And in another email, David describes the __local_fixups__ format
>> nicely, so I'll just copy that here instead of re-inventing it:
>>
>>
>>> Well, I don't want to invent a new encoding if we can possibly avoid
>>> it.  The current encoding used for overlay generation looks like this
>>>
>>> / {
>>> 	target: node@0 {
>>> 	};
>>> 	node@1 {
>>> 		ref = <&target>;
>>> 	};
>>> 	__local_fixups__ = {
>>> 		node@1 {
>>> 			ref = <0>;
>>> 		};
>>> 	};
>>> };
>>>
>>> Basically, __local_fixups__  has a subtree which paralells the main
>>> tree.  Each property found under __local_fixups__ is a list of offsets
>>> at which phandle references appear in the corresponding property in
>>> the main tree.
>>
>> I share your desire to rant about the different formats between
>> __local_fixups__ and __fixups__.  But I have not come up with an
>> alternate format for __local_fixups__ that makes me happy.  The
>> best format that I have come up with so far would be:
> 
> Well to fix it minimally, I'd go the other way - make __fixups__ look
> like __local_fixups__ but augmented with labels.  Strings that need
> parsing aren't a normal thing in the DT.

On the string parsing issue, I agree that string parsing is not normal
in the DT.  If changing format in other ways, I would maybe also change
the __fixups__ format so that (for an example with two tuples), instead
of

   "A:B:C", "D:E:F"

the format would be

   "A", "B", <C>, "D", "E", <F>.

Or a more concrete example, change:

   i2c1 = "/fragment@1:target:0";

to 

   i2c1 = "/fragment@1", "target", <0>;

or (to bikeshed) even change the order to:

   i2c1 = <0>, "/fragment@1", "target">;

This may look a little awkward in source form, but in my version
of what the world should look like, this would not be hand coded
in a DTS source file, but instead created by dtc in a DTB.  Of
course it could still be viewed as DTS format by de-compiling
the DTB.

I admit this may be a really bad idea from a human usability
standpoint, because the source fragment (for example):

        __fixups__ {
                i2c1 = <0>, "/fragment@1", "target";
                i2c2 = <8>, "/fragment@1", "target";
                i2c3 = "/fragment@1", "target", <0>;
                i2c4 = "/fragment@1", "target", <8>;
        };

decompiles (via 'dtc -O dts') somewhat cryptically as:

	__fixups__ {
		i2c1 = "", "", "", "", "/fragment@1", "target";
		i2c2 = "", "", "", "\b/fragment@1", "target";
		i2c3 = "/fragment@1", "target", "", "", "", "";
		i2c4 = [2f 66 72 61 67 6d 65 6e 74 40 31 00 74 61 72 67 65 74 00 00 00 00 08];
	};


-Frank

> 
>> / {
>> 	target: node@0 {
>> 	};
>> 	node@1 {
>> 		ref = <&target>;
>>                 ref2 = <&target 42 &target_2>;
>> 	};
>>         target_2: node@2 {
>>         };
>> 	__local_fixups__ = {
>> 		x1 = <"node@1/ref" 0>;
>>                 x2 = <"node@1/ref2" 0 8>;
>> 		};
>> 	};
>> };
>>
>> x1 and x2 are abitrary property names.
>> The format of each __local_fixups__ property is
>>    - path of property referencing a phandle
>>    - list of offsets of the phandle in the property
>>
>> As another alternative, Grant was thinking about adding
>> a new block to the FDT format to contain the phandle
>> information.  That would remove the need to come up
>> with a convoluted dts syntax, but adds in the problem
>> of bootloaders corrupting the new block if they were
>> not aware of it.  He had thoughts about versioning
>> and checksums to detect the corruption it if did
>> occur.
>>
>> If we were starting from scratch, do you have any other
>> approach that might be fruitful?  It seems like maybe
>> I am missing something that requires thinking outside
>> the box.
> 
> I thought about this the other day a bit.  If going from scratch, I
> think the way to do it would be to add a new FDT_REF tag to the
> structure block stream.  After the FDT_PROP tag and its contents,
> you'd have an arbitrary number of FDT_REF tags, each giving an offset
> in the preceding property  and a label to fix it up to match.  Not
> sure if you'd want separate FDT_REF and FDT_LOCAL_REF or just use an
> empty label to describe a local ref.
> 
> This would also allow for extension to say FDT_PATH_REF to insert
> paths rather than phandles (i.e. a runtime equivalent of prop = &foo;
> rather than prop = < &foo >;).
> 
> For encoding the fragments of an overlay, I'd suggest giving them
> simply as separate subtrees in the structure block, all before the
> FDT_END tag. At the moment there has to be only a single subtree
> before the FDT_END, and the top-level FDT_BEGIN is expected to have an
> empty name.  We can extend that to overlays by allowing multiple
> subtrees, and making the top-level "name" the target label instead.
> 
> Incidentally, I'd take "label" in all the above to be represented as
> an old-style OF path.  That is, either an absolute path /foo/bar/baz,
> or a path relative to an alias, alias/foo/bar/baz.  That means we can
> just use the existing defined /aliases, rather than re-inventing it as
> __symbols__.
>