Virtualization difficulty -- phandles

Virtualization difficulty -- phandles

[Cyril Novikov]

Hi, all!

The product that I work on supports physical device assignment (aka 
"pass-through") to virtual machines, which means we need to take 
portions of the physical FDT and include them in the guest FDT. This 
needs to happen automatically in software.

The problem is phandles, because we cannot identify them in the blob and 
therefore can't find any dependent devices/nodes that also need to be 
included in the guest FDT. So the process cannot be fully automated. We 
can't even advise the user what other devices should be assigned to the 
VM. The hypervisor runs or bare metal, so having to parse the source DTS 
files for this is very inconvenient.

Would it be possible to add metadata properties to the binary FDT 
format, which would identify other property cells that are in fact 
phandles? It could be a per-node property or a single root node 
property, up to you guys. DTC would then automatically generate the 
metadata property along with the phandle property when compiling the DTS.

--
Cyril

Re: Virtualization difficulty -- phandles

[Florian Fainelli]

On 07/11/2017 11:15 PM, Cyril Novikov wrote:
> Hi, all!
> 
> The product that I work on supports physical device assignment (aka
> "pass-through") to virtual machines, which means we need to take
> portions of the physical FDT and include them in the guest FDT. This
> needs to happen automatically in software.
> 
> The problem is phandles, because we cannot identify them in the blob and
> therefore can't find any dependent devices/nodes that also need to be
> included in the guest FDT. So the process cannot be fully automated. We
> can't even advise the user what other devices should be assigned to the
> VM. The hypervisor runs or bare metal, so having to parse the source DTS
> files for this is very inconvenient.
> 
> Would it be possible to add metadata properties to the binary FDT
> format, which would identify other property cells that are in fact
> phandles? It could be a per-node property or a single root node
> property, up to you guys. DTC would then automatically generate the
> metadata property along with the phandle property when compiling the DTS.

phandles are already per-node properties, they are not quite different
from normal properties actually. If a node is referred to by other
nodes, the DTC compiler would add a phandle = <N> (and maybe even
linux,phandle = <N> depending on options passed to it) where N is a
global integer that keeps being incremented every time a new phandle is
generated.

When you strip or create new nodes from a base blob for your virtual
machine, either the node is still existing, in which case its phandle
property (if existing) is still valid and can still be referenced to, or
it is a new node and then you can control how to allocate new phandle
integers.

I guess I am just not clear on what problem you are seeing with phandles
based on your description of what you want to do?
-- 
Florian

Re: Virtualization difficulty -- phandles

[Cyril Novikov]

On 7/12/2017 10:10 AM, Florian Fainelli wrote:
> On 07/11/2017 11:15 PM, Cyril Novikov wrote:
>> Hi, all!
>>
>> The product that I work on supports physical device assignment (aka
>> "pass-through") to virtual machines, which means we need to take
>> portions of the physical FDT and include them in the guest FDT. This
>> needs to happen automatically in software.
>>
>> The problem is phandles, because we cannot identify them in the blob and
>> therefore can't find any dependent devices/nodes that also need to be
>> included in the guest FDT. So the process cannot be fully automated. We
>> can't even advise the user what other devices should be assigned to the
>> VM. The hypervisor runs or bare metal, so having to parse the source DTS
>> files for this is very inconvenient.
>>
>> Would it be possible to add metadata properties to the binary FDT
>> format, which would identify other property cells that are in fact
>> phandles? It could be a per-node property or a single root node
>> property, up to you guys. DTC would then automatically generate the
>> metadata property along with the phandle property when compiling the DTS.
> 
> phandles are already per-node properties, they are not quite different
> from normal properties actually. If a node is referred to by other
> nodes, the DTC compiler would add a phandle = <N> (and maybe even
> linux,phandle = <N> depending on options passed to it) where N is a
> global integer that keeps being incremented every time a new phandle is
> generated.
> 
> When you strip or create new nodes from a base blob for your virtual
> machine, either the node is still existing, in which case its phandle
> property (if existing) is still valid and can still be referenced to, or
> it is a new node and then you can control how to allocate new phandle
> integers.
> 
> I guess I am just not clear on what problem you are seeing with phandles
> based on your description of what you want to do?

Maybe I should have worded it differently: the problem is with phandle 
references rather than phandle properties themselves, does it make it 
more clear? There is no way to know that a certain aligned 4 byte 
sequence is a phandle that references another part of the FDT. You can 
for some standard properties whose format is known not only to the 
driver, but you can't in general. That makes it impossible to analyze 
and detect dependencies between different parts of the FDT automatically.

I think this situation is solvable with automatically DTC-generated 
metadata. I'm also interested if there are other hypervisor vendors that 
had to deal with this and how big is the demand for a solution. If we 
are the first one, at least let it register that there's a problem and 
interest in addressing it.

--
Cyril

Re: Virtualization difficulty -- phandles

[Florian Fainelli]

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!
>>>
>>> The product that I work on supports physical device assignment (aka
>>> "pass-through") to virtual machines, which means we need to take
>>> portions of the physical FDT and include them in the guest FDT. This
>>> needs to happen automatically in software.
>>>
>>> The problem is phandles, because we cannot identify them in the blob and
>>> therefore can't find any dependent devices/nodes that also need to be
>>> included in the guest FDT. So the process cannot be fully automated. We
>>> can't even advise the user what other devices should be assigned to the
>>> VM. The hypervisor runs or bare metal, so having to parse the source DTS
>>> files for this is very inconvenient.
>>>
>>> Would it be possible to add metadata properties to the binary FDT
>>> format, which would identify other property cells that are in fact
>>> phandles? It could be a per-node property or a single root node
>>> property, up to you guys. DTC would then automatically generate the
>>> metadata property along with the phandle property when compiling the
>>> DTS.
>>
>> phandles are already per-node properties, they are not quite different
>> from normal properties actually. If a node is referred to by other
>> nodes, the DTC compiler would add a phandle = <N> (and maybe even
>> linux,phandle = <N> depending on options passed to it) where N is a
>> global integer that keeps being incremented every time a new phandle is
>> generated.
>>
>> When you strip or create new nodes from a base blob for your virtual
>> machine, either the node is still existing, in which case its phandle
>> property (if existing) is still valid and can still be referenced to, or
>> it is a new node and then you can control how to allocate new phandle
>> integers.
>>
>> I guess I am just not clear on what problem you are seeing with phandles
>> based on your description of what you want to do?
> 
> Maybe I should have worded it differently: the problem is with phandle
> references rather than phandle properties themselves, does it make it
> more clear?

It does, thanks.

> There is no way to know that a certain aligned 4 byte
> sequence is a phandle that references another part of the FDT. You can
> for some standard properties whose format is known not only to the
> driver, but you can't in general. That makes it impossible to analyze
> and detect dependencies between different parts of the FDT automatically.

I see what you mean now, there is indeed no way to tell whether a
property that has an integer is just a normal integer versus an integer
corresponding to a phandle.

You could argue that property that specify a phandle should have a name
that suggests so, like "phy-handle" but then this stops working with
e.g: gpios that are (at least with Linux) specified as e.g: reset-gpios.
In any case, you would have to have some sort of heuristic built into
your FDT mangling code that tries to check if a given property is
designating a phandle as opposed to having a more robust approach...

> 
> I think this situation is solvable with automatically DTC-generated
> metadata. I'm also interested if there are other hypervisor vendors that
> had to deal with this and how big is the demand for a solution. If we
> are the first one, at least let it register that there's a problem and
> interest in addressing it.

That would work, I have not heard of a similar problem with the
hypervisor folks that I worked with, but AFAIR they were generating
their DTS almost from scratch as opposed to mangling/passing through
parts of an existing one.
-- 
Florian

Re: Virtualization difficulty -- phandles

[Mark Rutland]

On Tue, Jul 11, 2017 at 11:15:55PM -0700, Cyril Novikov wrote:
> Hi, all!

Hi,

> The product that I work on supports physical device assignment (aka
> "pass-through") to virtual machines, which means we need to take
> portions of the physical FDT and include them in the guest FDT. This
> needs to happen automatically in software.
>
> The problem is phandles, because we cannot identify them in the blob
> and therefore can't find any dependent devices/nodes that also need
> to be included in the guest FDT. So the process cannot be fully
> automated. We can't even advise the user what other devices should
> be assigned to the VM. The hypervisor runs or bare metal, so having
> to parse the source DTS files for this is very inconvenient.

This has come up before in similar contexts (e.g. Xen platform device
pass-through). The simple answer is that this cannot be fully automated
unless your hypervisor has knowledge of all relevant bindings, such that
if can parse the FDT and make decisions based on this.

For better or worse the simple answer is that if a HV wants to assign
portions of HW to a guest, then the HV has to have an understanding of
that HW (and that HW's DT binding), or it has to trust some other agent
to configure things appropriately (setting up Stage-2 mappings,
providing a filtered FDT).

For example, you might have:

	clk: clock-controller {
		comptible = "vendor,clock-controller";
		...
		#clock-cells = <1>;
	};

	dev: device {
		compatible = "vendor,device";
		...
		clocks = <&clk 17>;
	};

... and you might want to pass dev through to a guest. You *cannot*
simply/safely add clk to that guest's DT (even with any stage-2 mappings
set up), since clk might (implicitly) feed other devices in the system,
and the guest might turn those clocks off, bringing down other guests,
or the hypervisor.

There are also cases like:

	singleton: some-singleton-device {
		...
	};

	dev: device {
		/* implicitly depends on singleton, but has no phandle */
	};

... for which it is necessary to have intimate knowledge of the dev
binding.

> Would it be possible to add metadata properties to the binary FDT
> format, which would identify other property cells that are in fact
> phandles? It could be a per-node property or a single root node
> property, up to you guys. DTC would then automatically generate the
> metadata property along with the phandle property when compiling the
> DTS.

Unfortunately, even ignoring the above, this metadata isn't likely to be
reliable, as after compilation, other agents (e.g. the bootloader) may
modify the FDT, without updating the metadata that they are not aware
of.

So either this would need to be a major (incompatible) change to the
format, or it would be necessary to have some mechanism to detect that
case, which leaves you at today's situation.

Thanks,
Mark.

Re: Virtualization difficulty -- phandles

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6237 bytes --]

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!
> >>>
> >>> The product that I work on supports physical device assignment (aka
> >>> "pass-through") to virtual machines, which means we need to take
> >>> portions of the physical FDT and include them in the guest FDT. This
> >>> needs to happen automatically in software.
> >>>
> >>> The problem is phandles, because we cannot identify them in the blob and
> >>> therefore can't find any dependent devices/nodes that also need to be
> >>> included in the guest FDT. So the process cannot be fully automated. We
> >>> can't even advise the user what other devices should be assigned to the
> >>> VM. The hypervisor runs or bare metal, so having to parse the source DTS
> >>> files for this is very inconvenient.
> >>>
> >>> Would it be possible to add metadata properties to the binary FDT
> >>> format, which would identify other property cells that are in fact
> >>> phandles? It could be a per-node property or a single root node
> >>> property, up to you guys. DTC would then automatically generate the
> >>> metadata property along with the phandle property when compiling the
> >>> DTS.
> >>
> >> phandles are already per-node properties, they are not quite different
> >> from normal properties actually. If a node is referred to by other
> >> nodes, the DTC compiler would add a phandle = <N> (and maybe even
> >> linux,phandle = <N> depending on options passed to it) where N is a
> >> global integer that keeps being incremented every time a new phandle is
> >> generated.
> >>
> >> When you strip or create new nodes from a base blob for your virtual
> >> machine, either the node is still existing, in which case its phandle
> >> property (if existing) is still valid and can still be referenced to, or
> >> it is a new node and then you can control how to allocate new phandle
> >> integers.
> >>
> >> I guess I am just not clear on what problem you are seeing with phandles
> >> based on your description of what you want to do?
> > 
> > Maybe I should have worded it differently: the problem is with phandle
> > references rather than phandle properties themselves, does it make it
> > more clear?
> 
> It does, thanks.
> 
> > There is no way to know that a certain aligned 4 byte
> > sequence is a phandle that references another part of the FDT.

In fact a phandle reference doesn't even have to be 4-byte aligned.

> You can
> > for some standard properties whose format is known not only to the
> > driver, but you can't in general. That makes it impossible to analyze
> > and detect dependencies between different parts of the FDT automatically.
> 
> I see what you mean now, there is indeed no way to tell whether a
> property that has an integer is just a normal integer versus an integer
> corresponding to a phandle.

Right.  Device tree properties are simply bytestrings until you use a
binding to interpret them.

> You could argue that property that specify a phandle should have a name
> that suggests so, like "phy-handle" but then this stops working with
> e.g: gpios that are (at least with Linux) specified as e.g: reset-gpios.

Not to mention that it doesn't help with properties which include
phandle references along with other information.

> In any case, you would have to have some sort of heuristic built into
> your FDT mangling code that tries to check if a given property is
> designating a phandle as opposed to having a more robust approach...

> > I think this situation is solvable with automatically DTC-generated
> > metadata. I'm also interested if there are other hypervisor vendors that
> > had to deal with this and how big is the demand for a solution. If we
> > are the first one, at least let it register that there's a problem and
> > interest in addressing it.
> 
> That would work, I have not heard of a similar problem with the
> hypervisor folks that I worked with, but AFAIR they were generating
> their DTS almost from scratch as opposed to mangling/passing through
> parts of an existing one.

So.  dtc in fact already has code to add metadata which marks phandle
references - so far it's only used in "plugin" files which are
intended to compile into overlays which can be runtime applied.  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).

I've had a request from someone else to add __local_fixups__
information in a non-plugin tree for yet another application, and it
would be pretty straightforward to implement that.

But.. I'm very uneasy about the idea.

The first thing, is that this relies on the dts file containing the
&whatever phandle reference syntax wherever there should be a phandle
reference.  That's normal, but nothing stops a user from simply
putting an actual number there, manually assigning that number to
another node's phandle and generating a valid dtb that way.

More importantly, it won't detect phandles if the tree comes from a
source other than a dts - for example if you flatten /proc/device-tree
with -I fs, or have code which flattens a tree presented by real Open
Firmware it won't have that phandle metadata, just integer values.

Now those might not be something that happens in your use case, but
I'm pretty concerned that if I added this functionality, people are
going to forget that properties are all, fundamentally, bytestrings
and get surprised when tools don't magically know where the phandles
are in some cases.

That said, I'm open to persuasion if a good enough case is made for
this functionality.


But then, as Mark Rutland says in his other replies, locating phandles
is far from the only problem in trying to passthrough random DT nodes
to a guest.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Virtualization difficulty -- phandles

[Cyril Novikov]

On 7/15/2017 10:35 PM, David Gibson wrote:

> But.. I'm very uneasy about the idea.
> 
> The first thing, is that this relies on the dts file containing the
> &whatever phandle reference syntax wherever there should be a phandle
> reference.  That's normal, but nothing stops a user from simply
> putting an actual number there, manually assigning that number to
> another node's phandle and generating a valid dtb that way.
> 
> More importantly, it won't detect phandles if the tree comes from a
> source other than a dts - for example if you flatten /proc/device-tree
> with -I fs, or have code which flattens a tree presented by real Open
> Firmware it won't have that phandle metadata, just integer values.

Yeah, so the metadata does not contain full information. That's fine 
because it's still better than nothing.

> Now those might not be something that happens in your use case, but
> I'm pretty concerned that if I added this functionality, people are
> going to forget that properties are all, fundamentally, bytestrings
> and get surprised when tools don't magically know where the phandles
> are in some cases.
> 
> That said, I'm open to persuasion if a good enough case is made for
> this functionality.

So, what we need fundamentally is a way to detect dependencies in the 
devicetree. Mark is arguing that we cannot *automate* the assignment of 
dependent devices to the same VM. However, just detecting that there is 
a missing dependency goes a long way. What we have now is guest drivers 
failing in various manners, but what they cannot do is point the user to 
the specific device that they need, because they don't have the host FDT 
in their VM.

For example, a driver may print something like

[    2.035071] xuartps ff000000.serial: pclk clock not found.

To resolve this, the user needs to be proficient with the device tree. 
Of course, "pclk" is not a node name in the host FDT. However, if 
configuration software could detect this at the time the set of the VM's 
assigned physical devices was created, it could give the user the name 
of the device that *probably* also needs to be assigned to the same VM. 
The user wouldn't need to learn anything about devicetree, they would 
just need to do a few mouse clicks to assign the dependent device. And 
now it's the software's concern if that assignment is not trivial, as 
Mark warns us.

To summarize, full automation is unfeasible and it is understood, but 
just the dependency information is very helpful too.

> But then, as Mark Rutland says in his other replies, locating phandles
> is far from the only problem in trying to passthrough random DT nodes
> to a guest.

--
Cyril

Re: Virtualization difficulty -- phandles

[Cyril Novikov]

On 7/14/2017 3:58 AM, Mark Rutland wrote:

>> Would it be possible to add metadata properties to the binary FDT
>> format, which would identify other property cells that are in fact
>> phandles? It could be a per-node property or a single root node
>> property, up to you guys. DTC would then automatically generate the
>> metadata property along with the phandle property when compiling the
>> DTS.
> 
> Unfortunately, even ignoring the above, this metadata isn't likely to be
> reliable, as after compilation, other agents (e.g. the bootloader) may
> modify the FDT, without updating the metadata that they are not aware
> of.

Well, it depends on the design of said metadata. For example, imagine a 
node property named ".dependencies", which simply lists all phandles 
referenced by other properties in the same node. I would be very 
surprised if the bootloader does something that breaks that. It would 
require a major intervention into the FDT.

Even if they do, like I said in my reply to David, we don't strive to 
achieve a full automation, so it's probably tolerable. Oh well, we've 
missed some dependencies. It's still better than what we have now.

--
Cyril

Re: Virtualization difficulty -- phandles

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3113 bytes --]

On Mon, Jul 17, 2017 at 06:37:47PM -0700, Cyril Novikov wrote:
> On 7/15/2017 10:35 PM, David Gibson wrote:
> 
> > But.. I'm very uneasy about the idea.
> > 
> > The first thing, is that this relies on the dts file containing the
> > &whatever phandle reference syntax wherever there should be a phandle
> > reference.  That's normal, but nothing stops a user from simply
> > putting an actual number there, manually assigning that number to
> > another node's phandle and generating a valid dtb that way.
> > 
> > More importantly, it won't detect phandles if the tree comes from a
> > source other than a dts - for example if you flatten /proc/device-tree
> > with -I fs, or have code which flattens a tree presented by real Open
> > Firmware it won't have that phandle metadata, just integer values.
> 
> Yeah, so the metadata does not contain full information. That's fine because
> it's still better than nothing.

Hm, ok.

> > Now those might not be something that happens in your use case, but
> > I'm pretty concerned that if I added this functionality, people are
> > going to forget that properties are all, fundamentally, bytestrings
> > and get surprised when tools don't magically know where the phandles
> > are in some cases.
> > 
> > That said, I'm open to persuasion if a good enough case is made for
> > this functionality.
> 
> So, what we need fundamentally is a way to detect dependencies in the
> devicetree. Mark is arguing that we cannot *automate* the assignment of
> dependent devices to the same VM. However, just detecting that there is a
> missing dependency goes a long way. What we have now is guest drivers
> failing in various manners, but what they cannot do is point the user to the
> specific device that they need, because they don't have the host FDT in
> their VM.

Makes sense.

> For example, a driver may print something like
> 
> [    2.035071] xuartps ff000000.serial: pclk clock not found.
> 
> To resolve this, the user needs to be proficient with the device tree. Of
> course, "pclk" is not a node name in the host FDT. However, if configuration
> software could detect this at the time the set of the VM's assigned physical
> devices was created, it could give the user the name of the device that
> *probably* also needs to be assigned to the same VM. The user wouldn't need
> to learn anything about devicetree, they would just need to do a few mouse
> clicks to assign the dependent device. And now it's the software's concern
> if that assignment is not trivial, as Mark warns us.
> 
> To summarize, full automation is unfeasible and it is understood, but just
> the dependency information is very helpful too.

Ok, sounds plausible.

> > But then, as Mark Rutland says in his other replies, locating phandles
> > is far from the only problem in trying to passthrough random DT nodes
> > to a guest.
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Virtualization difficulty -- phandles

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

On Mon, Jul 17, 2017 at 06:47:07PM -0700, Cyril Novikov wrote:
> On 7/14/2017 3:58 AM, Mark Rutland wrote:
> 
> > > Would it be possible to add metadata properties to the binary FDT
> > > format, which would identify other property cells that are in fact
> > > phandles? It could be a per-node property or a single root node
> > > property, up to you guys. DTC would then automatically generate the
> > > metadata property along with the phandle property when compiling the
> > > DTS.
> > 
> > Unfortunately, even ignoring the above, this metadata isn't likely to be
> > reliable, as after compilation, other agents (e.g. the bootloader) may
> > modify the FDT, without updating the metadata that they are not aware
> > of.
> 
> Well, it depends on the design of said metadata. For example, imagine a node
> property named ".dependencies", which simply lists all phandles referenced
> by other properties in the same node. I would be very surprised if the
> bootloader does something that breaks that. It would require a major
> intervention into the FDT.

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.

That seems like it would survive most likely bootloader
transformations as well, I think.

> Even if they do, like I said in my reply to David, we don't strive to
> achieve a full automation, so it's probably tolerable. Oh well, we've missed
> some dependencies. It's still better than what we have now.

Ok.  I'm tentatively convinced that it's worth adding a switch to dtc
to generate __local_fixups__even for a non-plugin source.

Next step is for someone to propose a concrete patch.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Virtualization difficulty -- phandles

[Cyril Novikov]

On 7/18/2017 8:40 PM, David Gibson wrote:

> 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.
> 
> That seems like it would survive most likely bootloader
> transformations as well, I think.
> 
>> Even if they do, like I said in my reply to David, we don't strive to
>> achieve a full automation, so it's probably tolerable. Oh well, we've missed
>> some dependencies. It's still better than what we have now.
> 
> Ok.  I'm tentatively convinced that it's worth adding a switch to dtc
> to generate __local_fixups__even for a non-plugin source.
> 
> Next step is for someone to propose a concrete patch.

Do you mean a patch to the DTC utility or a patch to the Devicetree 
Specification? I suppose we want this specced, so that alternative 
incompatible implementations do not appear? The engineer looking at it 
told me it's only a few lines change for the DTC.

Re: Virtualization difficulty -- phandles

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]

On Fri, Jul 21, 2017 at 09:24:35PM -0700, Cyril Novikov wrote:
> On 7/18/2017 8:40 PM, David Gibson wrote:
> 
> > 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.
> > 
> > That seems like it would survive most likely bootloader
> > transformations as well, I think.
> > 
> > > Even if they do, like I said in my reply to David, we don't strive to
> > > achieve a full automation, so it's probably tolerable. Oh well, we've missed
> > > some dependencies. It's still better than what we have now.
> > 
> > Ok.  I'm tentatively convinced that it's worth adding a switch to dtc
> > to generate __local_fixups__even for a non-plugin source.
> > 
> > Next step is for someone to propose a concrete patch.
> 
> Do you mean a patch to the DTC utility or a patch to the Devicetree
> Specification?

I was meaning a patch to dtc.  Exactly what's mean by the "Devicetree
Specification" is a bit unclear.  There's the spec of the flattened
encoding, the spec of what goes in the tree (i.e. the collected
bindings).  And then there's the spec of formats using the flattened
tree format to encode something that's not a traditional device tree
(though it might be related), like FIT and overlays.

> I suppose we want this specced, so that alternative
> incompatible implementations do not appear?

Well sort of, but it's not clear where to put it.  This is basically a
new option to make dtc add "overlay style" __local_fixups__
information to a non-overlay.  Does that belong in the overlay spec
(such as it is) or somewhere else.

> The engineer looking at it told
> me it's only a few lines change for the DTC.

Maybe a little more than that to make something polished, but not
complex, no.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Virtualization difficulty -- phandles

[Frank Rowand]

Hi Cyril,

On 07/11/17 23:15, Cyril Novikov wrote:
> Hi, all!
> 
> The product that I work on supports physical device assignment (aka
> "pass-through") to virtual machines, which means we need to take
> portions of the physical FDT and include them in the guest FDT. This
> needs to happen automatically in software.

Is this for Linux, or some other OS?

If Linux, do you get the FDT from /sys/firmware/fdt or some other
method?

-Frank

> The problem is phandles, because we cannot identify them in the blob
> and therefore can't find any dependent devices/nodes that also need
> to be included in the guest FDT. So the process cannot be fully
> automated. We can't even advise the user what other devices should be
> assigned to the VM. The hypervisor runs or bare metal, so having to
> parse the source DTS files for this is very inconvenient.
> 
> Would it be possible to add metadata properties to the binary FDT
> format, which would identify other property cells that are in fact
> phandles? It could be a per-node property or a single root node
> property, up to you guys. DTC would then automatically generate the
> metadata property along with the phandle property when compiling the
> DTS.
> 
> -- Cyril
> 
> -- To unsubscribe from this list: send the line "unsubscribe
> devicetree-spec" in the body of a message to
> majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html .
> 

Re: Virtualization difficulty -- phandles

[Frank Rowand]

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:

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

-Frank

Re: Virtualization difficulty -- phandles

[Cyril Novikov]

On 7/24/2017 9:27 AM, Frank Rowand wrote:
> Hi Cyril,
> 
> On 07/11/17 23:15, Cyril Novikov wrote:
>> Hi, all!
>>
>> The product that I work on supports physical device assignment (aka
>> "pass-through") to virtual machines, which means we need to take
>> portions of the physical FDT and include them in the guest FDT. This
>> needs to happen automatically in software.
> 
> Is this for Linux, or some other OS?
> 
> If Linux, do you get the FDT from /sys/firmware/fdt or some other
> method?

No, this is for a hypervisor running on bare metal, not under Linux. We 
get the device tree from the board vendor with their Linux port.

--
Cyril

Re: Virtualization difficulty -- phandles

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4532 bytes --]

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.

> / {
> 	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__.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Virtualization difficulty -- phandles

[Frank Rowand]

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

Re: Virtualization difficulty -- phandles

[Pantelis Antoniou]

Hi Frank,

On Thu, 2017-07-27 at 13:58 -0700, Frank Rowand wrote:
> 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];
> 	};
> 
> 

The format of the fixups was originally proposed as it was precisely for
this reason. Yes, parsing strings are icky but parsing strings
interspersed with integers is even ickier.

This problem would be easily solvable if there was a method to record
type information about the sequence of property elements. You would
never even need fixups.

For example you could generate a shadow property for each property that
is encountered and fill it with information about the type of the
original named property.

For instance for ref2 = <&target 42 &target_2> you could generate a
.ref2 = "rcl" property that encodes <remote-reference>, <cell>,
<local-reference>.

It would be pretty efficient too since the second property can a)
eliminated in most cases when the auto type detection (like the one used
in fdtdump) is used and b) the name contains the original property name
so it can be reused in the string table.

You wouldn't need the __fixups__ nodes at all then. The original ref2
property would be encoded in a way that the value placed in the place of
the &target be an offset in the string table that would contain the name
of the remote reference to resolve.

Regards

-- Pantelis

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

Re: Virtualization difficulty -- phandles

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6726 bytes --]

On Thu, Jul 27, 2017 at 01:58:09PM -0700, Frank Rowand wrote:
> 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">;

Right, but by re-using the parallel paths encoding from
__local_fixups__ you can also drop the path element, simplifying this
a bit further.

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]