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

[Heinrich Schuchardt]

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.

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

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

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.

> +		fdt_pages = 0;

This line is not really needed as our if-statements don't use it.

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 */