Hi Grant I am not also in favor to do DTB update like this patch , but DTB update is needed feature !! Ideally DTB defined once should be done forever but unfortunately we are not in ideal world. More inline please Regards Udit > -----Original Message----- > From: Grant Likely [mailto:grant.likely@xxxxxxx] > Sent: Wednesday, April 18, 2018 5:03 PM > To: Udit Kumar <udit.kumar@xxxxxxx>; 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>; nd@xxxxxxx; Rod Dorris <rod.dorris@xxxxxxx> > Subject: Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration > table > > 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 Let's start using ACPI then 😊 , With DTB we are going to hit similar problems > kernel is a bad model for supporting distributions. 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. DTB are defining hardware in bit and byte basics, this is difficult to have full responsibility of DTB by kernel e.g. platform supports psci or not this is known to firmware as firmware created needed infrastructure for the same. There are many other examples, which will say DTB shouldn't be owned by kernel. But having under firmware, we need to see how to update DTB. > Allowing firmware to modify a kernel-supplied DTB can result in side effects that > cannot be overridden by the dtb= kernel parameter. I guess, no one here prefer to update dtb in this way. > >> 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. Ideally we shouldn’t practically we need to > 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. Agreed, we need to define, that with given OS update some notification if DTB updates are needed or not > 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. Sure, hope in EBBR we can find some standard way > > > > > >> * 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. Say platform is shipped with kernel 4.4 and associated DTBs. Now user wants to upgrade to kernel 4.9 , for sure platform should boot at same level of functions as of 4.4. In new kernel, added DT bindings are agreed with Linux community, But old DTB enhanced feature cannot be used, Then what is point to update to new OS e.g iommu-map is added for few devices (https://github.com/qoriq-open-source/linux/blob/linux-4.9/arch/arm64/boot/dts/freescale/fsl-ls208xa.dtsi) > > > >> 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. Ok > g. ��.n��������+%������w��{.n�����{����* jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
