On 18 April 2018 at 13:32, Grant Likely <grant.likely@xxxxxxx> wrote: > On 18/04/2018 09:44, Udit Kumar wrote: >> >> >> >>> -----Original Message----- >>> From: Grant Likely [mailto:grant.likely@xxxxxxx] >>> Sent: Monday, March 26, 2018 4:10 PM >>> To: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>; Pankaj Bansal >>> <pankaj.bansal@xxxxxxx>; mark.rutland@xxxxxxx; leif.lindholm@xxxxxxxxxx >>> Cc: linux-efi@xxxxxxxxxxxxxxx; Varun Sethi <V.Sethi@xxxxxxx>; Wasim Khan >>> <wasim.khan@xxxxxxx>; Udit Kumar <udit.kumar@xxxxxxx>; nd@xxxxxxx >>> Subject: Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in >>> configuration >>> table >>> >>> On 25/03/2018 16:29, Ard Biesheuvel wrote: >>>> >>>> >>>>> On 25 Mar 2018, at 14:56, Pankaj Bansal <pankaj.bansal@xxxxxxx> wrote: >>>>> >>>>> Bootloader may need to fixup the device tree before OS can use it. >>>>> e.g. a UEFI/DXE driver that has initialized a controller can add >>>>> controller's clock frequency in controller node. This way OS need not >>>>> to call get/set clock for that controller. >>>>> >>>>> Therefore, install fdt used by OS in configuration tables and >>>>> associate it with device tree guid. >>>>> >>>> >>>> The firmware should be providing the dtb in the first place. On a uefi >>>> system, >>> >>> you should not use the dtbs shipped with the kernel, or use the dtb= >>> command >>> line parameter. >>>> >>>> >>>> This may sound overly strict, but it is the only maintainable way: the >>>> dt bindings are the contract between firmware and os, and the os >>>> shipped dtbs essentially implement the platform side of the contract, >>>> pushing the firmware-os boundary down to a layer that is entirely >>>> unstandardized. Exposing the bundled dtb to the firmware is fragile, >>>> since the firmware has no guarantees whatsoever about its contents. >>>> (dt bindings are [supposed to be] backward compatible, but the device >>>> trees themselves are not) >>>> >>>> So please find another way to solve this, and move away from dtb= >>> >>> >>> Completely agree here. I do agree that it is sometimes important to >>> override the >>> firmware provided data. We've got precidence for doing this with ACPI >>> also. >>> However, this approach is something different entirely. >> >> >> Sorry to come back late on this topic, >> With ACPI, we have clear abstraction on code location and ownership. >> Normally device tree are copied from kernel and build into UEFI , This is >> different situation here >> where code location is different than ownership. >> This is unrelated but u-boot ITB boot, everything is assumed external to >> boot-loader >> Kernel and device tree are tied together. > > > That's the model we're trying to break away from. Having the DTB tied to the > kernel is a bad model for supporting distributions. I'd say it is a bad model period. This requires the OS to know about every piece of hardware this kernel could potentially ever run on, and not only does this not scale, it also means you have to predict the future if you are ever going try and run an OS that is older than the hardware it runs on. Having a well defined contract (i.e., the DT bindings) was designed to remove these impediments, but in reality, the tight coupling of kernel and DT resulted in bad developer hygiene when it comes to honoring the contract in a backward compatible manner. > I want to make sure the > model supports both: > 1) DTB responsibilty of firmware - in which case the kernel either uses it > or ignores it; but the kernel cannot provide a new dtb that the firmware > manipulates > 2) DTB responsibility of the kernel - in which case the firmware won't > modify the DTB at all. > > Allowing firmware to modify a kernel-supplied DTB can result in side effects > that cannot be overridden by the dtb= kernel parameter. > It /could/ work if the bindings are the only a priori contract between the firmware and the bootloader, but given the corner we have already painted ourselves into by shipping DTs with the kernel images, this is highly likely to make things worse rather than better. >>> This patch tries to take a kernel-provided .dtb, register it *back with >>> firmware* so that firmware can modify it. That is a completely different >>> contract, and not one that should be encouraged. >> >> >>> If firmware provides a DTB, then the acceptable options are either accept >>> that >>> DTB as is, or completely replace it*, with a strong preference for >>> trusting the >>> firmware provided DTB. If the upstream kernel breaks with the firmware- >>> provided DTB, then that is a kernel bug and should be fixed.** >> >> >> Well, kernel must boot even with old device tree But say some driver is >> adding new feature >> then with old device tree, this feature may not be used. >> Therefore device-tree update and needed, and platform specific information >> needs to be >> fixed before handing over to OS > > > I don't disagree with needing to update the DTB, but I disagree with the > approach. > > If a DTB update is needed, then the right model is to update the DTB in > firmwre *before* going to the DTB boot stage. We should create a standard > interface for doing this, but it should not be part of the the kernel UEFI > stub, and it should not magically change the DTB based on the kernel's dtb= > parameters line. > > The firmware DTB update could be either temporary or persistant, but it > needs to be explicitly a separate step from the kernel boot. Exactly what > this looks like can be discussed in the EBBR call. > Updating your firmware should be so easy and hassle free that it really does not matter if you are updating the entire image or just the DTB. I don't think there should be a mandated way to update the DT only if you can just rebuild your firmware including the DT and update the whole thing. >> >> >>> * I'm not talking about overlays here. That's an extension scenario >>> which builds on what firmware provides instead of replaces it. >>> ** Two notes on this; >>> 1) it is okay to require a firmware dtb update to enable new >>> functionality, but it is not okay for the kernel to break >>> things that work. >> >> >> For first part I agree and second is not understood my me ' but it is not >> okay for the kernel to break things that work' >> Using kernel provided device tree and fixing by UEFI, why you think kernel >> will break the things. > > > I only mean that once a platform ships with a DTB embedded in firmware > (presuming that DTB will boot a mainline kernel), future releases of that > kernel need to continue supporting that platform DTB with the same level of > functionality. We've not been good at that in the past, but if a kernel > update breaks booting, that is a kernel bug and it needs to be fixed. > Yes. This implies that you cannot modify a DT binding without versioning it and retaining support for the old versions. >> >>> 2) We really should have a standard way in Tianocore to update >>> the built in DTB without reflashing the firmware image. >> >> >> We need to find a better way for DTB updating , for sure above method >> should be avoided. >> Can capsule update be used here ? > > > Possibly. I'd like to make sure the U-Boot UEFI implementation can use > capsule updates. > > g. > -- > To unsubscribe from this list: send the line "unsubscribe linux-efi" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
