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

[Sughosh Ganu]

On Wed, 26 Mar 2025 at 13:54, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> 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()

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