> -----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. > 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'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. > 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 ? > > Cheers, > g. > > > > > > > > >> UEFI/DXE drivers can fixup this device tree in their respective > >> ExitBootServices events. > >> > >> Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx> > >> Cc: linux-efi@xxxxxxxxxxxxxxx > >> Signed-off-by: Pankaj Bansal <pankaj.bansal@xxxxxxx> > >> --- > >> drivers/firmware/efi/libstub/fdt.c | 8 ++++++++ > >> 1 file changed, 8 insertions(+) > >> > >> diff --git a/drivers/firmware/efi/libstub/fdt.c > >> b/drivers/firmware/efi/libstub/fdt.c > >> index 177654e..df862e6 100644 > >> --- a/drivers/firmware/efi/libstub/fdt.c > >> +++ b/drivers/firmware/efi/libstub/fdt.c > >> @@ -265,6 +265,7 @@ efi_status_t > allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table_arg, > >> int runtime_entry_count = 0; > >> struct efi_boot_memmap map; > >> struct exit_boot_struct priv; > >> + efi_guid_t fdt_guid = DEVICE_TREE_GUID; > >> > >> map.map = &runtime_map; > >> map.map_size = &map_size; > >> @@ -314,6 +315,13 @@ efi_status_t > allocate_new_fdt_and_exit_boot(efi_system_table_t *sys_table_arg, > >> goto fail_free_new_fdt; > >> } > >> > >> + status = efi_call_early(install_configuration_table, &fdt_guid, > >> + (void *)*new_fdt_addr); > >> + if (status != EFI_SUCCESS) { > >> + pr_efi_err(sys_table_arg, "Unable to install new device tree.\n"); > >> + goto fail_free_new_fdt; > >> + } > >> + > >> priv.runtime_map = runtime_map; > >> priv.runtime_entry_count = &runtime_entry_count; > >> priv.new_fdt_addr = (void *)*new_fdt_addr; > >> -- > >> 2.7.4 > >> ��.n��������+%������w��{.n�����{����* jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
