Re: Virtualization difficulty -- phandles

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

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