On Wed, 26 Mar 2025 at 13:54, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> 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()
I had actually thought about this, but did not put it in the
efi_install_fdt() as we have a case for GENERATE_ACPI_TABLE, where we
are not actually doing anything with the device-tree. So copy_fdt() is
the place where we know that we are going to install the DT as
configuration table, so then remove the existing configuration table.
>
> 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
For the above scenario, we will not be getting any other error than
EFI_NOT_FOUND no? If you have a strong opinion on this, I can make the
change. Please let me know. Thanks.
-sughosh
>
> 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
> >
