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

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@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(-)

Hi Sughosh

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

On Wed, 26 Mar 2025 at 13:54, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Sughosh
>
> On Mon, 17 Mar 2025 at 10:34, Sughosh Ganu <sughosh.ganu@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@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
> >

[...]

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

Fair enough, leave it as is. I'll queue the patches shortly

Thanks
/Ilias

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

The config table is there it will remove successfully 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
> > >