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

[Ilias Apalodimas]

Hi Sughosh

On Mon, 17 Mar 2025 at 10:34, Sughosh Ganu <sughosh.ganu at linaro.org> 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 a
> device-tree is already installed as an EFI configuration
> table. However, this existing copy of the 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 V8:
> * Re-word the commit message to indicate DT being installed as EFI
>   configuration table
> * Remove the existing EFI config table in copy_fdt()
> * Move assignment of new_fdt_addr and fdt_pages variables to the block
>   freeing up the existing config table memory
>
>  lib/efi_loader/efi_helper.c | 39 +++++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 10 deletions(-)
>
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 15ad042bc61..f6fbcdffe82 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -454,11 +454,30 @@ 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;
> +
> +       /*
> +        * Remove the configuration table that might already be
> +        * installed, ignoring EFI_NOT_FOUND if no device-tree
> +        * is installed
> +        */
> +       efi_install_configuration_table(&efi_guid_fdt, NULL);
> +
> +       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);
> +               if (ret != EFI_SUCCESS)
> +                       log_err("Unable to free up existing FDT memory region\n");
> +
> +               new_fdt_addr = 0;
> +               fdt_pages = 0;
> +       }
>
>         /*
>          * Give us at least 12 KiB of breathing room in case the device tree
> @@ -473,15 +492,18 @@ static efi_status_t copy_fdt(void **fdtp)
>                                  &new_fdt_addr);
>         if (ret != EFI_SUCCESS) {
>                 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 +556,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;

This is the last nit I have an I think these are ready for  -next.

Instead of removing the existing table in copy_fdt, can we remove it
before calling copy_fdt() in efi_install_fdt()

if (efi_get_configuration_table(&efi_guid_fdt))
    efi_install_configuration_table(&efi_guid_fdt, NULL);
should be enough and you can also check the return code that way

Cheers
/Ilias


>                 /* Check if there is a hardware device tree */
>                 fdt_opt = env_get("fdt_addr");
>                 /* Use our own device tree as fallback */
> --
> 2.34.1
>