[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 at google.com> wrote:
>>
>> Hi Heinrich,
>>
>> On Mon, 23 Oct 2023 at 23:20, Heinrich Schuchardt
>> <heinrich.schuchardt at 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 at 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 at chromium.org>
>>
>> Regards,
>> SImon