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. > However, the core of Thiery's point, AFAICT, is that that won't matter > because overlays won't know about these phandles anyway. > > Thiery is basically right, at least in the case where this is only > being used on a "full" dtb - that is either a base dtb (before > overlays are applied) or a dtb to which overlays have already been > applied. It will break if you attempt to use this directly on an > overlay dtb that has not yet been applied to something, since a valid > overlay *must* have metadata for everywhere a phandle appears within > it. > > "Full" trees however (before or after applying overlays) only need > metadata for nodes which later overlays might want to reference. The > phandle values they contain are final. Yes. > So, for Thiery's usecase - and potentiually a bunch of others - it > should be fine and I think it's reasonable to merge this. > It might be > worth adding a warning to the doc comments saying that the function > isn't safe to use on a dtbo (well, technically it's storing the > generated phandle back into the overlay anywhere that's unsafe). That is a point that I had not even considered. Yes, dynamically adding a phandle and adding a use of the phandle value in the dtbo will not work. When an overlay is loaded all of the locations using the phandle value have the phandle value adjusted by the overlay loader. If the phandle metadata is not present, then the required adjustments will not be done.
