[PATCH v8 2/8] efi_loader: install device-tree on configuration table on every invocation

[Heinrich Schuchardt]

On 13.03.25 08:14, Sughosh Ganu wrote:
> On Thu, 13 Mar 2025 at 01:40, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> wrote:
>>
>> On 12.03.25 09:54, Sughosh Ganu wrote:
>>> The efi_install_fdt() function is called before booting an EFI binary,
>>> either directly, or through a bootmanager. This function installs a
>>> copy of the device-tree(DT) on the EFI configuration table, which is
>>> passed on to the OS.
>>>
>>> The current logic in this function does not install a DT if the table
>>> already has a DT installed on it. However, this existing copy of the
>>
>> The device-tree is the configuration table. Maybe better write:
>>
>> ... if a device-tree is already installed as an EFI configuration table.
> 
> Oh. Isn't the device-tree *one* of the entries on the configuration
> table? I mean, the spec says this, "The EFI Configuration table is the
> ConfigurationTable field in the EFI System Table. This table contains
> a set of GUID/pointer pairs". And one of these pairs happens to be the
> DT.


The UEFI spec uses the term "configuration table" both for the field 
ConfigurationTable in the SystemTable as well as for the objects that 
this table is pointing to.

UEFI specification 2.11, Chapter 7.5.6, 
EFI_BOOT_SERVICES.InstallConfigurationTable(), has this sentence:

The InstallConfigurationTable() function is used to maintain the list of 
configuration tables that are stored in the EFI System Table.

So the device-tree is one of the configuration tables like the SMBIOS 
table, and many others.

As the specification uses the term "configuration table" with 
alternative significations, it would be preferable if the commit message 
would make it clear which one is being referred to.

Best regards

Heinrich

> 
>>
>>> DT might not be up-to-date, or it could be a wrong DT for the image
>>> that is being booted. Always install a DT afresh to the configuration
>>> table before booting the EFI binary.
>>>
>>> Installing a new DT also involves some additional checks that are
>>> needed to clean up memory associated with the existing DT copy. Check
>>> for an existing copy, and free up that memory.
>>>
>>> Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
>>> ---
>>> Changes since V7:
>>> * Change the type of fdt_pages to efi_uintn_t
>>> * Remove the check for either of fdt_pages or new_fdt_addr being 0
>>> * Change the format specifier for fdt_pages in the debug messages for
>>>     size_t
>>> * Put the assignment of new_fdt_addr and fdt_pages on separate lines
>>>     to avoid checkpatch error
>>>
>>>
>>>    lib/efi_loader/efi_helper.c | 32 ++++++++++++++++++++++----------
>>>    1 file changed, 22 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
>>> index 15ad042bc61..37e5859741f 100644
>>> --- a/lib/efi_loader/efi_helper.c
>>> +++ b/lib/efi_loader/efi_helper.c
>>> @@ -454,11 +454,21 @@ efi_status_t efi_env_set_load_options(efi_handle_t handle,
>>>     */
>>>    static efi_status_t copy_fdt(void **fdtp)
>>>    {
>>> -     unsigned long fdt_pages;
>>>        efi_status_t ret = 0;
>>>        void *fdt, *new_fdt;
>>> -     u64 new_fdt_addr;
>>> -     uint fdt_size;
>>> +     static u64 new_fdt_addr;
>>> +     static efi_uintn_t fdt_pages;
>>> +     ulong fdt_size;
>>> +
>>> +     if (new_fdt_addr) {
>>> +             log_debug("%s: Found allocated memory at %#llx, with %#zx pages\n",
>>> +                       __func__, new_fdt_addr, fdt_pages);
>>> +             ret = efi_free_pages(new_fdt_addr, fdt_pages);
>>
>> We should not free the memory before removing the configuration table.
>> Otherwise in an error situation we might end up with an invalid
>> device-tree pointer.
> 
> Yes, that is a good point. Will add this change.
> 
>>
>> /* Ignoring EFI_NOT_FOUND if not device-tree is installed */
>> efi_install_configuration_table(&efi_guid_fdt, NULL);
>> if (new_fdt_addr) {
>>          ret = efi_free_pages(new_fdt_addr, fdt_pages);
>>          ...
>>
>>> +             if (ret != EFI_SUCCESS) {
>>> +                     log_err("Unable to free up existing FDT memory region\n");
>>   > +                    return ret;> +          }
>>
>> Ok, something weird happened. But this should not stop booting.
> 
> Okay, so I now see that none of the EFI code seems to be checking the
> return value of the efi_free_{pages, pool} functions.
> 
>>
>> Just set new_fdt_addr = 0 in any case.
>>
>>          if (ret!= EFI_SUCCES)
>>                  log_err("Failed to free existing FDT memory\n");
>>          new_fdt_addr = 0;
>>
>>> +     }
>>>
>>>        /*
>>>         * Give us at least 12 KiB of breathing room in case the device tree
>>> @@ -472,16 +482,21 @@ static efi_status_t copy_fdt(void **fdtp)
>>>                                 EFI_ACPI_RECLAIM_MEMORY, fdt_pages,
>>>                                 &new_fdt_addr);
>>>        if (ret != EFI_SUCCESS) {
>>> +             new_fdt_addr = 0;
>>
>> efi_allocate_pages() only updates new_fdt_addr() in case of success.
>> If we set new_fdt_addr = 0 above, this line might be removed.
> 
> Okay
> 
>>
>>> +             fdt_pages = 0;
>>
>> This line is not really needed as our if-statements don't use it.
> 
> Yeah, but it just feels weird just zeroing out one of the parameters
> and not the other. Someone who reads this code later might find it
> odd. And I am sure that the compiler would be doing the necessary
> optimisations. If you don't have a strong opinion on this, I would
> prefer this stay.
> 
> -sughosh
> 
>>
>> Best regards
>>
>> Heinrich
>>
>>>                log_err("Failed to reserve space for FDT\n");
>>> -             goto done;
>>> +             return ret;
>>>        }
>>> +     log_debug("%s: Allocated memory at %#llx, with %#zx pages\n",
>>> +               __func__, new_fdt_addr, fdt_pages);
>>> +
>>>        new_fdt = (void *)(uintptr_t)new_fdt_addr;
>>>        memcpy(new_fdt, fdt, fdt_totalsize(fdt));
>>>        fdt_set_totalsize(new_fdt, fdt_size);
>>>
>>> -     *fdtp = (void *)(uintptr_t)new_fdt_addr;
>>> -done:
>>> -     return ret;
>>> +     *fdtp = new_fdt;
>>> +
>>> +     return EFI_SUCCESS;
>>>    }
>>>
>>>    /**
>>> @@ -534,9 +549,6 @@ efi_status_t efi_install_fdt(void *fdt)
>>>                const char *fdt_opt;
>>>                uintptr_t fdt_addr;
>>>
>>> -             /* Look for device tree that is already installed */
>>> -             if (efi_get_configuration_table(&efi_guid_fdt))
>>> -                     return EFI_SUCCESS;
>>>                /* Check if there is a hardware device tree */
>>>                fdt_opt = env_get("fdt_addr");
>>>                /* Use our own device tree as fallback */
>>