Re: [PATCH 2/2] drivers: firmware: efi: install new fdt in configuration table

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

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.**

* 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.
  2) We really should have a standard way in Tianocore to update
     the built in DTB without reflashing the firmware image.

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


--
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



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux