On 10/25/23 20:23, Simon Glass wrote:
Hi Heinrich,

On Tue, 24 Oct 2023 at 18:02, Simon Glass <s...@google.com> wrote:

Hi Heinrich,

On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
<heinrich.schucha...@canonical.com> wrote:

Forward and backward compatibility of Linux kernel device-trees is
sometimes missing. One solution approach is to load a kernel specific
device-tree. This can either be done via a U-Boot scripts (like the one
generated by Debian package flash-kernel or by a boot loader like GRUB.
The boot loader approach currently requires to know the device-tree name
before first boot which makes it unusable for generic images.

Expose the device-tree file name as EFI variable FdtFile.
This will allow bootloaders to load a kernel specific device-tree.

kernel-specific


The variable will not be exposed on ACPI based systems or if the
environment variable fdtfile is not defined.

Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com>
---
v4:
         Generalize the description of the content of $fdtfile.
v3:
         Add documentation
v2:
         Use a unique GUID to enable future U-Boot independent
         standardization.
         Do not try to add the variable on ACPI based systems.
---
  doc/develop/uefi/uefi.rst  | 14 ++++++++++++++
  include/efi_loader.h       |  5 +++++
  lib/efi_loader/efi_setup.c | 30 ++++++++++++++++++++++++++++++
  3 files changed, 49 insertions(+)

diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
index fb16ac743a..702c490831 100644
--- a/doc/develop/uefi/uefi.rst
+++ b/doc/develop/uefi/uefi.rst
@@ -916,6 +916,20 @@ So our final format of the FilePathList[] is::

      Loaded image - end node (0xff) - VenMedia - initrd_1 - [end node (0x01) - 
initrd_n ...] - end node (0xff)

+EFI variable FdtFile
+~~~~~~~~~~~~~~~~~~~~
+
+Ideally U-Boot would always expose a device-tree that can be used for booting
+any operating systems. Unfortunately operating systems like Linux sometimes
+break forward and backward compatibility. In this case there is a need to load
+an operating system version specific device-tree.

This seems to be a strong statement. Given the effort that goes into
the DT, changes are supposed to be backwards-compatible. Is this
generally true, or is it just that we want an up-to-date DT for the
kernel to enable new features?

Did you see this comment?

It would have been nice to put the person which made that comment on copy.

The truth lies in the world "supposed":

The idea of a device-tree that never needs to change is quite old and never became true on ARM devices.

We all know Linux tends to break both forward and backward compatibility of device-trees. Here is a nice example:

d0c6707ca423 ("arm64: dts: allwinner: H5: NanoPi Neo Plus2: phy-mode rgmii-id")

Driver changes broke forward and backwards compatibility of a lot of Allwinner boards.

Distros will continue to load the device-tree that matches the kernel to get the best possible board support and need to do this efficiently.

Best regards

Heinrich


Regards,
Simon


+
+U-Boot has an environment variable fdtfile identifying the device-tree file to
+load. The content of this variable is exposed as EFI variable Fdtfile, vendor
+GUID d45dde69-3bd6-40e0-90d5-6b606aa57730. It contains the device-tree path
+name as a NUL terminated ASCII string. On many architectures the file name is

NUL-terminated

+preceded by a vendor directory ('vendor-directory/board-name.dtb').
+
  Links
  -----

diff --git a/include/efi_loader.h b/include/efi_loader.h
index e24410505f..146e7f1bce 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -152,6 +152,11 @@ static inline efi_status_t efi_launch_capsules(void)
         EFI_GUID(0x8108ac4e, 0x9f11, 0x4d59, \
                  0x85, 0x0e, 0xe2, 0x1a, 0x52, 0x2c, 0x59, 0xb2)

+/* Vendor GUID for the FdtFile variable */
+#define VENDOR_FDTFILE_GUID \
+       EFI_GUID(0xd45dde69, 0x3bd6, 0x40e0, \
+                0x90, 0xd5, 0x6b, 0x60, 0x6a, 0xa5, 0x77, 0x30)
+
  /* Use internal device tree when starting UEFI application */
  #define EFI_FDT_USE_INTERNAL NULL

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index e6de685e87..71bcde645b 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -17,6 +17,8 @@

  efi_status_t efi_obj_list_initialized = OBJ_LIST_NOT_INITIALIZED;

+efi_guid_t vendor_fdtfile_guid = VENDOR_FDTFILE_GUID;
+
  /*
   * Allow unaligned memory access.
   *
@@ -26,6 +28,27 @@ void __weak allow_unaligned(void)
  {
  }

+/**
+ * efi_init_fdtfile() - set EFI variable FdtFile
+ *
+ * Return:     status code
+ */
+static efi_status_t efi_init_fdtfile(void)
+{
+       char *val;
+
+       val = env_get("fdtfile");
+       if (!val)
+               return EFI_SUCCESS;
+
+       return efi_set_variable_int(u"FdtFile",
+                                   &vendor_fdtfile_guid,
+                                   EFI_VARIABLE_BOOTSERVICE_ACCESS |
+                                   EFI_VARIABLE_RUNTIME_ACCESS |
+                                   EFI_VARIABLE_READ_ONLY,
+                                   strlen(val) + 1, val, false);
+}
+
  /**
   * efi_init_platform_lang() - define supported languages
   *
@@ -250,6 +273,13 @@ efi_status_t efi_init_obj_list(void)
         if (ret != EFI_SUCCESS)
                 goto out;

+       /* Define EFI variable FdtFile */
+       if (!CONFIG_IS_ENABLED(GENERATE_ACPI_TABLE)) {
+               ret = efi_init_fdtfile();
+               if (ret != EFI_SUCCESS)
+                       goto out;
+       }
+
         /* Indicate supported features */
         ret = efi_init_os_indications();
         if (ret != EFI_SUCCESS)
--
2.40.1


Assuming my concerns above are figured out:

Reviewed-by: Simon Glass <s...@chromium.org>

Regards,
SImon

Reply via email to