On 25/03/2018 16:29, Ard Biesheuvel wrote:

On 25 Mar 2018, at 14:56, Pankaj Bansal <[email protected]> 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 <[email protected]>
Cc: [email protected]
Signed-off-by: Pankaj Bansal <[email protected]>
---
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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to