On Mon, Apr 08, 2019 at 10:13:12AM +0200, Thierry Reding wrote: > On Thu, Apr 04, 2019 at 02:27:36PM +1100, David Gibson wrote: > > On Wed, Apr 03, 2019 at 01:18:38PM -0700, Frank Rowand wrote: > > > On 3/28/19 7:51 PM, David Gibson wrote: > > > > On Mon, Mar 25, 2019 at 09:18:24AM +0100, Thierry Reding wrote: > > > >> On Fri, Mar 22, 2019 at 03:49:56AM -0700, Frank Rowand wrote: > > > >>> On 3/22/19 1:58 AM, Thierry Reding wrote: > > > >>>> On Thu, Mar 21, 2019 at 11:37:20AM -0700, Frank Rowand wrote: > > > >>>>> On 3/21/19 3:26 AM, Thierry Reding wrote: > > > >>>>>> On Wed, Mar 20, 2019 at 05:36:15PM -0700, Frank Rowand wrote: > > > >>>>>>> On 3/20/19 8:10 AM, Thierry Reding wrote: > > > >>>>>>>> From: Thierry Reding <treding@xxxxxxxxxx> > > > >>>>>>>> > > > >>>>>>>> The new fdt_generate_phandle() function can be used to generate a new, > > > >>>>>>>> unused phandle given a specific device tree blob. The implementation is > > > >>>>>>>> somewhat naive in that it simply walks the entire device tree to find > > > >>>>>>>> the highest phandle value and then returns a phandle value one higher > > > >>>>>>>> than that. A more clever implementation might try to find holes in the > > > >>>>>>>> current set of phandle values and fill them. But this implementation is > > > >>>>>>>> relatively simple and works reliably. > > > >>>>>>>> > > > >>>>>>>> Also add a test that validates that phandles generated by this new API > > > >>>>>>>> are indeed unique. > > > >>>>>>> > > > >>>>>>> I don't see any issues. > > > >>>>>>> > > > >>>>>>> I am curious as to what the envisioned use is for fdt_generate_phandle(). > > > >>>>>> > > > >>>>>> The idea here is to allow firmware to pass information about carveout > > > >>>>>> areas to the Linux kernel (and potentially other OSes). To specify the > > > >>>>>> carveouts, it needs to create subnodes in the top-level /reserved-memory > > > >>>>>> node and attach a phandle to those nodes so that they can later be > > > >>>>>> referenced by users of the carveouts. > > > >>>>>> > > > >>>>>> One such example would be a framebuffer carveout set up by early > > > >>>>>> firmware to show a boot splash. In order to prevent the OS kernel from > > > >>>>>> recycling the framebuffer carveout for its allocator, the firmware needs > > > >>>>>> to marks it as reserved memory. This is what the child nodes in the top- > > > >>>>>> level /reserved-memory node do. Then the display controller that's > > > >>>>>> scanning out the framebuffer from that carveout needs to be passed a > > > >>>>>> reference to the reserved memory node (via its phandle, which is stored > > > >>>>>> in the display controller node's memory-region property). > > > >>>>>> > > > >>>>>> The display controller driver can then use that memory-region property > > > >>>>>> to look up the reserved memory region and reuse it, copy out data, ... > > > >>>>>> This information can also be used on early OS boot to setup things like > > > >>>>>> IOMMU page tables so that there's initially a 1:1 mapping of physical > > > >>>>>> to IO virtual addresses for the carveout. This is necessary because the > > > >>>>>> display controller may otherwise cause a lot of IOMMU faults trying to > > > >>>>>> access a physical memory address through the IOMMU. > > > >>>>>> > > > >>>>>> Does that clarify the use-case? > > > >>>>> > > > >>>>> Yes, thanks. That is an excellent description. > > > >>>>> > > > >>>>> I think that it is not good to provide a mechanism to create phandles > > > >>>>> in libfdt, because people are likely to use the mechanism if it > > > >>>>> exists. How is that for a rationale? :-) > > > >>>>> > > > >>>>> The problem is that such a phandle is entirely outside the "architecture" > > > >>>>> (and I use that word loosely) of phandle related infrastructure used > > > >>>>> for overlays. And I am not just thinking of current implementation, but > > > >>>>> also thinking of how I am hoping the implementation will change. > > > >>>>> > > > >>>>> Either the phandle overlay related metadata will not be available, or > > > >>>>> fdt_generate_phandle() would have to add the complication of also > > > >>>>> creating that metadata (which I also think is not a good idea). > > > >>>> > > > >>>> I'm not sure I understand how that's relevant to this discussion. DT > > > >>>> overlays and phandles are completely orthogonal. phandles need to work > > > >>>> without knowing anything about overlays. > > > >>> > > > >>> If an overlay is going to be applied on top of a base FDT, and the overlay > > > >>> references a phandle in the base FDT, then the base FDT has to have been > > > >>> compiled with the '-@' option so that extra metadata describing the > > > >>> phandles will be included in the base FDT. Without the extra metadata, > > > >>> the overlay can not be applied. > > > >>> > > > >>> If an overlay needs to reference the phandle created by fdt_generate_phandle() > > > >>> then the overlay apply will fail because the metadata is not in the base > > > >>> FDT. > > > >> > > > >> I have a hard time imagining how this is supposed to work. If the > > > >> phandle is generated at runtime, how will anyone know how to reference > > > >> it in an overlay? Unless they generate the overlay at runtime, in which > > > >> case why bother with the overlay mechanism at all? > > > >> > > > >>>>> Is there an alternative approach to achieve the same required use-case? > > > >>>>> One possibility that jumped to mind (but I haven't thought through yet) > > > >>>>> is to put the possible carveout area definitions in the base dts source, > > > >>>>> but with a size of zero, where size zero means not active. Then the > > > >>>>> firmware could update the size (and offset if needed) to create the > > > >>>>> actual carveout. > > > >>>> > > > >>>> That's not very practical. First of all, we can't assume that the DTB > > > >>>> can be changed on every device that needs to add carveouts. Also, it is > > > >>>> not necessarily known at the time that the DTS is compiled that there > > > >>>> will even be carveouts. Having them occupy space in the device tree if > > > >>>> they are never used is wasteful and easily confusing. > > > >>> > > > >>> If the base DTB contains a node for a display driver that is capable of > > > >>> detecting a framebuffer carveout, then it is not unlikely that the > > > >>> carveout will exist. It is not especially confusing to have an unused > > > >>> carveout if there is an unambiguous indication of an inactive carveout > > > >>> (such as size zero). We already handle inactive nodes that are > > > >>> marked as such by the status property. > > > >> > > > >> I'm not sure what you mean by base DTB, but there are a number of > > > >> reasons why there may be a device tree node for a display controller but > > > >> no need for the carveout. For example a DTB may list a node for a > > > >> display controller, but the display controller may end up not being used > > > >> if the user didn't connect a monitor. Or a user may want to just disable > > > >> the display controller (or not compile/load the driver) if they want to > > > >> run the device in headless mode. > > > >> > > > >> Secondly, bootloaders may not implement display support at all, so even > > > >> if it's possible to output something on a screen and users have > > > >> connected a screen, the bootloader may just not support it. So the > > > >> existence of the carveout is dependent on so many things that it's a bit > > > >> of a stretch to require the DTS file to contain carveouts for every > > > >> possible scenario. One other thing to consider in this context is that a > > > >> carveout is not hardware description, so a lot of DTBs exist that don't > > > >> contain any such information. This means that extending bootloaders with > > > >> functionality that require the creation of a carveout would effectively > > > >> be a device tree ABI break. > > > >> > > > >> Thirdly the carveout will often be allocated dynamically. While we can > > > >> fill in any values at runtime, in order to create a "conformant" DTB we > > > >> would also need to rename the nodes so that they can contain the proper > > > >> unit-addresses. This all ends up with pretty much the same code as we'd > > > >> need to generate all of the nodes from scratch. > > > >> > > > >> Lastly there's also the issue that I've seen bootloaders fail to deal > > > >> with carveouts that have zero size. You could argue that that's a bug in > > > >> the bootloader implementation, but then again, I don't think it's a big > > > >> stretch for users to assume that if there is no reserved memory there's > > > >> no need to have a node at all, so not handling the zero-size carveout > > > >> case is something that I'd think is fairly common. > > > >> > > > >>>> Also, I don't understand how that would change anything with regards to > > > >>>> your point above about overlay metadata. Surely just because the DTS is > > > >>>> compiled doesn't guarantee that overlay metadata will be produced. > > > >>> > > > >>> Correct, the metadata is only in the DTB if the proper compile flag is > > > >>> specified. But if the carveout (with phandle) is in the DTS and the > > > >>> compile flag is specified, then dtc will create the metadata for that > > > >>> phandle. > > > >> > > > >> Like I said, I don't think this is really an issue that will happen. My > > > >> understanding is that overlays are built on top of a base DTB that needs > > > >> to be (or at least usually will be) available at compile time. Users of > > > >> fdt_generate_phandle() will only ever create the phandles way after the > > > >> creation of DTBs or overlays, so I don't think this would be an issue. > > > >> > > > >> Even if there was an issue, there's nothing preventing us from just > > > >> rejecting both to work together. You already said that an overlay can't > > > >> be applied if there's no metadata for a given phandle. So we already > > > >> have these checks in place and people will just not be able to do this. > > > >> > > > >> You're arguing that we should not be adding support for generating > > > >> phandle values at runtime because it may break in one specific use-case. > > > >> That's a bit like saying people shouldn't be allowed to wear shoes > > > >> because they may trip after stepping on their own shoelaces. > > > > > > > > So.. I think you're talking at cross purposes a bit here. > > > > > > Yes. > > > > > > And in addition, I'm still trying to grok how much the boot > > > loadersmodify a DTB before passing it on to the kernel. I would > > > like to become more aware of what sort of processing by the boot > > > loaders already occurs. > > > > > > > > > > The core of Frank's point, AFAICT, is that using this code would > > > > generate phandles which don't have corresponding symbols / fixups > > > > information for overlay application, which might be dangerous. > > > > > > Basically yes. But not dangerous, just prevents overlays from using > > > the generated phandles. > > > > Oh, ok. That doesn't seem like such a big deal to me. Overlays can > > already only use phandles for nodes that are labelled (well, more or > > less). > > > > If we need this, then, yes, we'd need more support in libfdt. > > Something like fdt_add_label() which would allocate a phandle and also > > add in the necessary symbol information. > > > > Functions like that are a bit nasty for the libfdt API, though, > > because it makes a non-local change to the tree, any offsets you might > > have in local variables will be invalidated. > > I've run into this occasionally. I've been working on a patch series > that uses this phandle generation helper to create carveouts for the > display framebuffers. One of the problems I ran into early on was that > after adding a new subnode into the /reserved-memory node, the offset > I had previously looked up for the display controller's device tree node > was pointing at a completely different node. > > My workaround for that was not to pass in an offset in the first place, > but instead pass in the path to the display controller's device tree so > that the offset could be looked up from the path after the non-local > change was applied. > > I've been wondering whether there's a better solution for that, but I > suspect that short of something like using a live tree that gets written > back to an FDT after all modifications have taken place, this just isn't > possible. Basically, yes. There's not really any way to implement true persistent handles to nodes or properties on a flat tree. Or at least not without adding a large "context" structure. That would be both very difficult to implement without using dynamic memory allocation (which I want to avoid for embedded cases), and against the spirit of being able to use libfdt with minimal setup to make quick adjustments to the tree. Incidentally, this is why so many of the libfdt functions explicitly mention "offset". It's this way rather than saying just "node" or whatever, in the (perhaps vain) hope that it will remind people these are offsets which can be affected by other changes, rather than persistent handles. The saving grace here is that altering something within a node won't affect that node's offset, or that of any of its ancestor nodes. That is an interface guarantee of libfdt. The trouble with something like fdt_add_label() is that it could alter the offsets even for for the node you're working on, because we don't know where it sits in the blob w.r.t. the symbols node. It'd be nice to find time to make up the "live" dt library that Peter Maydell suggested (amongst others). Maybe that would discourage people from using libfdt for things more complex than it was ever really intended for. -- 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:
signature.asc
Description: PGP signature
