> -----Original Message----- > From: Grant Likely [mailto:grant.likely@xxxxxxx] > Sent: Tuesday, April 24, 2018 4:11 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 15:26, Udit Kumar wrote: > > Hi Grant > > > I am not also in favor to do DTB update like this patch , but DTB > update is needed feature !! > > Oh! Yes. I 100% agree. I am not arguing against DTB updates.. > > > Ideally DTB defined once should be done forever but unfortunately we are not > in ideal world. > > Also agree. This isn't about whether or not DTB updates are required. Of > course they are! However, there needs to be a clear division of > responsibilities. There are basically two scenarios we need to support: > > 1) Firmware provides DTB > In this case the firmware has a copy of the DTB, which I may modify at > boot time and then passes to the kernel. That DTB might be replaced with > a different version, or it might have overlays applied, but regardless > the firmware is entirely responsible for the DTB. We can have tooling in > firmware to make it easy to manage the DTB, including loading a newer > DTB, but that step must be distinct from the kernel's OS loader stub. IMO, firmware should be owner for DTB in UEFI system. To update or replace it, we can discuss the ways to follow. > 2) Kernel provides/overrides DTB > The kernel always has the option of loading it's own DTB, either because > firmware doesn't provide one, or it needs a newer DTB. This is similar > to CONFIG_ACPI_CUSTOM_DSDT in ACPI land. However, if the kernel is > overriding the DTB, then firmware has no part of it, and it should not > be allowed to modify the DTB at ExitBootServices() time. For our platforms at least, this will not work unless firmware is doing modifications. I think this I likely true for Ard as well. Firmware has a logic to modify default DTB for available devices, reading board configuration programming required muxes, based upon this removing/adding devices into DTB. IMO, this will not be an optimal way to use untouched DTB provided by kernel. > Similarly, Grub might decide to provide the override DTB instead of the > kernel, but that is effectively the same scenario. If the OS boot > process overrides the platform description, then the firmware no longer > has any part of it. I see same problem as of kernel providing DTB. > The second case is by far the most common scenario today, but it is > unfriendly to distributions, which is why it is important to change the > behaviour. Here is how I think it should be designed: > - Firmware should have a built-in DTB that will boot a mainline kernel > - Firmware should provide mechanism for installing updated DTB > - Reflashing entire firmware just for DTB updates is discouraged > - DTB update could be either temporary or persistant > - If persistent, DTB should be stored in firmware flash region > - ie. not in system partitition. > - Firmware can require DTB update to be signed > - Built-in DTB remains as failsafe that can always recovered to > - Firmware remains 'responsible' for DTB > - Firmware allowed to modify updated DTB > - Firmware update mechanism logically separate from OS boot > - Though it could be scripted by Grub > - Firmware 'vendor' responsible for official updates to DTB > - Question: should there be a standard capsule update mechanism for > DTB updates? > - By default, OS distributions should use firmware provided DTB > - Require user intervention DTB override is needed I see, this in two ways 1/ Replacement of DTB is needed, In this case, my preference will be to update whole firmware. 2/ Update in DTB is needed. I think above could be as above, In addition, I see - OS is providing DTB update as DTBO for given distribution - Which could lend as runtime variable in firmware. - Firmware is checking that and storing on flash with give Id - Boot-loader (grub) is checking for runtime variable for DTBO for given distribution - grub is merging firmware provided (DTB) and DTBO before handing over to OS > However, if the DTB update mechanism is standard, and if we have a > common database of DTBs, it would be possible for the distribution to > automatically manage DTB updates. This way , will not be standard for non-UEFI boot . > > > > 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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub > .com%2Fqoriq-open-source%2Flinux%2Fblob%2Flinux- > 4.9%2Farch%2Farm64%2Fboot%2Fdts%2Ffreescale%2Ffsl- > ls208xa.dtsi&data=02%7C01%7Cudit.kumar%40nxp.com%7Cb8ed0a1f67634a8a > 105708d5a9cfdd08%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C1%7C63 > 6601632606164363&sdata=qk7ECT7i7fIhzdVazOrkAKesS%2BbGmENuQaq%2B66 > hEU%2B8%3D&reserved=0) > > > > > >>> > >>>> 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�����٥
