[PATCH v4 1/1] efi_loader: expose the device-tree file name

[PATCH v4 1/1] efi_loader: expose the device-tree file name

[Heinrich Schuchardt]

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.

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.schuchardt@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.
+
+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
+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

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Simon Glass]

Hi Heinrich,

On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
<heinrich.schuchardt@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.schuchardt@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?

> +
> +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 <sjg@chromium.org>

Regards,
SImon

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 897 bytes --]

On Tue, Oct 24, 2023 at 08:20:32AM +0200, Heinrich Schuchardt 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.
> 
> 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.schuchardt@canonical.com>

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Ilias Apalodimas]

On Tue, 24 Oct 2023 at 09:20, Heinrich Schuchardt
<heinrich.schuchardt@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.
>
> 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.schuchardt@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.
> +
> +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
> +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
>

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Ilias Apalodimas]

On Tue, 24 Oct 2023 at 09:20, Heinrich Schuchardt
<heinrich.schuchardt@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.
>
> 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.schuchardt@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.
> +
> +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
> +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
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Simon Glass]

Hi Heinrich,

On Tue, 24 Oct 2023 at 18:02, Simon Glass <sjg@google.com> wrote:
>
> Hi Heinrich,
>
> On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
> <heinrich.schuchardt@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.schuchardt@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?

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 <sjg@chromium.org>
>
> Regards,
> SImon

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Heinrich Schuchardt]

On 10/25/23 20:23, Simon Glass wrote:
> Hi Heinrich,
> 
> On Tue, 24 Oct 2023 at 18:02, Simon Glass <sjg@google.com> wrote:
>>
>> Hi Heinrich,
>>
>> On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
>> <heinrich.schuchardt@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.schuchardt@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 <sjg@chromium.org>
>>
>> Regards,
>> SImon

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4500 bytes --]

On Wed, Oct 25, 2023 at 09:57:44PM +0200, Heinrich Schuchardt wrote:
> On 10/25/23 20:23, Simon Glass wrote:
> > Hi Heinrich,
> > 
> > On Tue, 24 Oct 2023 at 18:02, Simon Glass <sjg@google.com> wrote:
> > > 
> > > Hi Heinrich,
> > > 
> > > On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
> > > <heinrich.schuchardt@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.schuchardt@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.

Right, OK. And I think we want to try and have things phrased in a more
neutral and less confrontational manner is part of the issue. Maybe:

In ideal circumstances, U-Boot will be able to expose the device-tree it
is using to boot any operation system. However, in some cases operating
systems need to load a specific device-tree rather than utilize the same
one that U-Boot is currently using. In this case there is a need to load
a specific device-tree binary from another location.

And as a more general concern I see right now, "fdt_file" and "fdtfile"
are both used today, including new rather than older platforms that
might avoid EFI_LOADER all the same, perhaps we should check for both?
Or do you instead want to get board maintainers to switch over, as
fdt_file isn't listed under doc/ today.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Mark Kettenis]

> Date: Wed, 25 Oct 2023 21:57:44 +0200
> From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> 
> On 10/25/23 20:23, Simon Glass wrote:
> > Hi Heinrich,
> > 
> > On Tue, 24 Oct 2023 at 18:02, Simon Glass <sjg@google.com> wrote:
> >>
> >> Hi Heinrich,
> >>
> >> On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
> >> <heinrich.schuchardt@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.schuchardt@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.

Well, that happened in 2020.  Things have gotten better over time.

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

Right.  Even if there is full backward/forward compatibility, you
probably want the latest device-tree to make sure you get the most
complete hardware support.

But this shouldn't be used as an argument to not care about
backward/forward compatibility.

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Heinrich Schuchardt]

On 10/25/23 22:28, Mark Kettenis wrote:
>> Date: Wed, 25 Oct 2023 21:57:44 +0200
>> From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>
>> On 10/25/23 20:23, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Tue, 24 Oct 2023 at 18:02, Simon Glass <sjg@google.com> wrote:
>>>>
>>>> Hi Heinrich,
>>>>
>>>> On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
>>>> <heinrich.schuchardt@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.schuchardt@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.
> 
> Well, that happened in 2020.  Things have gotten better over time.

The 2020 event struck me because a Linux kernel would not even be 
compatible with the device-tree of the same kernel release.

This year I once again had issues with booting an Allwinner board with a 
device-tree from a different Debian kernel version. Both backward and 
forward compatibility were broken. But at least I could boot with a 
matching device-tree.

> 
>> 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.
> 
> Right.  Even if there is full backward/forward compatibility, you
> probably want the latest device-tree to make sure you get the most
> complete hardware support.
> 
> But this shouldn't be used as an argument to not care about
> backward/forward compatibility.

Nobody in this thread suggested to not care about forward and backward 
compatibility.

Best regards

Heinrich

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Simon Glass]

Hi Heinrich,

On Wed, 25 Oct 2023 at 20:51, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 10/25/23 22:28, Mark Kettenis wrote:
> >> Date: Wed, 25 Oct 2023 21:57:44 +0200
> >> From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>
> >> On 10/25/23 20:23, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Tue, 24 Oct 2023 at 18:02, Simon Glass <sjg@google.com> wrote:
> >>>>
> >>>> Hi Heinrich,
> >>>>
> >>>> On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
> >>>> <heinrich.schuchardt@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.schuchardt@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.
> >
> > Well, that happened in 2020.  Things have gotten better over time.
>
> The 2020 event struck me because a Linux kernel would not even be
> compatible with the device-tree of the same kernel release.
>
> This year I once again had issues with booting an Allwinner board with a
> device-tree from a different Debian kernel version. Both backward and
> forward compatibility were broken. But at least I could boot with a
> matching device-tree.

Oh dear.

>
> >
> >> 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.
> >
> > Right.  Even if there is full backward/forward compatibility, you
> > probably want the latest device-tree to make sure you get the most
> > complete hardware support.
> >
> > But this shouldn't be used as an argument to not care about
> > backward/forward compatibility.
>
> Nobody in this thread suggested to not care about forward and backward
> compatibility.

The key thing we need to agree on is that the compatible string is the
source of truth for booting. You check the model compatible and search
for a matching DT with the best match, taking into account board
revisions, etc. It is defined to be correct. If for some reason this
process is wrong, then the board *should not boot*.

So anything which suggests that the above is not correct, or doesn't
always work, or doesn't work well with EFI...is really going in the
wrong direction.

Of course there will be workarounds, like this one, but the 'correct'
way of doing it must still work. Nor is it 'slow' to do this, as I
have demonstrated.

Perhaps we should add a compatible check into U-Boot, to make sure we
are not choosing the wrong DT? At least for me, trying to get the
filename right is a bit of a pain, with so many characters looking the
same.

Regards,
Simon

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 4370 bytes --]

On Wed, Oct 25, 2023 at 10:28:05PM +0200, Mark Kettenis wrote:
> > Date: Wed, 25 Oct 2023 21:57:44 +0200
> > From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > 
> > On 10/25/23 20:23, Simon Glass wrote:
> > > Hi Heinrich,
> > > 
> > > On Tue, 24 Oct 2023 at 18:02, Simon Glass <sjg@google.com> wrote:
> > >>
> > >> Hi Heinrich,
> > >>
> > >> On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
> > >> <heinrich.schuchardt@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.schuchardt@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.
> 
> Well, that happened in 2020.  Things have gotten better over time.

Well, yes and no.  Given the brief summary here, I bet this was just
like when phy-mode and am335x platforms had DT compatibility broken and
the answer was that it was OK because the DT was incorrectly describing
hardware. So this is the reminder that there are cases of breaking DT
compatibility that are allowed. Even if the DT has been out (and wrong)
for several years.

That's not the main point of this thread and I don't want to derail
things further along this point, I just want to note that the details
here reminded me of when things are allowed to be incompatible with
previous trees.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Heinrich Schuchardt]

On 10/25/23 23:13, Tom Rini wrote:
> On Wed, Oct 25, 2023 at 10:28:05PM +0200, Mark Kettenis wrote:
>>> Date: Wed, 25 Oct 2023 21:57:44 +0200
>>> From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>
>>> On 10/25/23 20:23, Simon Glass wrote:
>>>> Hi Heinrich,
>>>>
>>>> On Tue, 24 Oct 2023 at 18:02, Simon Glass <sjg@google.com> wrote:
>>>>>
>>>>> Hi Heinrich,
>>>>>
>>>>> On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
>>>>> <heinrich.schuchardt@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.schuchardt@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.
>>
>> Well, that happened in 2020.  Things have gotten better over time.
> 
> Well, yes and no.  Given the brief summary here, I bet this was just
> like when phy-mode and am335x platforms had DT compatibility broken and
> the answer was that it was OK because the DT was incorrectly describing
> hardware. So this is the reminder that there are cases of breaking DT
> compatibility that are allowed. Even if the DT has been out (and wrong)
> for several years.
> 
> That's not the main point of this thread and I don't want to derail
> things further along this point, I just want to note that the details
> here reminded me of when things are allowed to be incompatible with
> previous trees.
> 

You are conflating things:

The EFI variable is a hint to the GRUB OS to find the correct 
device-tree file without scanning hundreds of device-tree. You can not 
build a compatibility check for it into U-Boot.

In distro-boot we are using the same value from environment variable 
$fdtfile. Here you could check the compatible string but this might 
break booting .

Best regards

Heinrich

Re: [PATCH v4 1/1] efi_loader: expose the device-tree file name

[Simon Glass]

Hi Heinrich,

On Wed, 25 Oct 2023 at 15:22, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 10/25/23 23:13, Tom Rini wrote:
> > On Wed, Oct 25, 2023 at 10:28:05PM +0200, Mark Kettenis wrote:
> >>> Date: Wed, 25 Oct 2023 21:57:44 +0200
> >>> From: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>
> >>> On 10/25/23 20:23, Simon Glass wrote:
> >>>> Hi Heinrich,
> >>>>
> >>>> On Tue, 24 Oct 2023 at 18:02, Simon Glass <sjg@google.com> wrote:
> >>>>>
> >>>>> Hi Heinrich,
> >>>>>
> >>>>> On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
> >>>>> <heinrich.schuchardt@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.schuchardt@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.
> >>
> >> Well, that happened in 2020.  Things have gotten better over time.
> >
> > Well, yes and no.  Given the brief summary here, I bet this was just
> > like when phy-mode and am335x platforms had DT compatibility broken and
> > the answer was that it was OK because the DT was incorrectly describing
> > hardware. So this is the reminder that there are cases of breaking DT
> > compatibility that are allowed. Even if the DT has been out (and wrong)
> > for several years.
> >
> > That's not the main point of this thread and I don't want to derail
> > things further along this point, I just want to note that the details
> > here reminded me of when things are allowed to be incompatible with
> > previous trees.
> >
>

Unfortunately I was off the list for a week or so, but I see this one
so will reply here.

> You are conflating things:
>
> The EFI variable is a hint to the GRUB OS to find the correct
> device-tree file without scanning hundreds of device-tree. You can not
> build a compatibility check for it into U-Boot.

Actually we can...and I think that is what we should do.

>
> In distro-boot we are using the same value from environment variable
> $fdtfile. Here you could check the compatible string but this might
> break booting .

We need to figure that point out. The compatible string is
programmatically is defined to be correct, so it is the filename that
may/may not be useful. What am I missing?

For distro-boot, do you mean the scripts? We are trying to deprecate
those. Of course we have brought in all the same work-arounds, etc.,
but with bootstd we can start to clean things up, I hope.

So let's think about how we can have U-Boot choose the right DT to boot with.

Regards,
Simon