Hi Sughosh
On Mon, 17 Mar 2025 at 10:34, Sughosh Ganu <sughosh.g...@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.g...@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
>
