On 4/3/19 8:27 PM, 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. > Agreed. This would add a level of complexity that I do not like.
