diff mbox series

[4/4] efi: add helper functions to insert pmem node for DT fixup

Message ID 20241025111411.165904-5-sughosh.ganu@linaro.org
State New
Headers show
Series Add pmem node for preserving distro ISO's | expand

Commit Message

Sughosh Ganu Oct. 25, 2024, 11:14 a.m. UTC
The EFI HTTP boot puts the iso installer image at some location in
memory which needs to be reserved in the devicetree as persistent
memory (pmem). Add helper functions which add this pmem node when the
EFI_DT_FIXUP protocol's fixup callback is invoked.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---
 boot/image-fdt.c             |  9 +++++++++
 include/efi_loader.h         | 17 +++++++++++++++++
 lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
 lib/efi_loader/efi_helper.c  | 12 ++++++++++++
 4 files changed, 59 insertions(+)

Comments

Ilias Apalodimas Oct. 25, 2024, 11:17 a.m. UTC | #1
+CC Anton

On Fri, 25 Oct 2024 at 14:14, Sughosh Ganu <sughosh.ganu@linaro.org> wrote:
>
> The EFI HTTP boot puts the iso installer image at some location in
> memory which needs to be reserved in the devicetree as persistent
> memory (pmem). Add helper functions which add this pmem node when the
> EFI_DT_FIXUP protocol's fixup callback is invoked.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>  boot/image-fdt.c             |  9 +++++++++
>  include/efi_loader.h         | 17 +++++++++++++++++
>  lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
>  lib/efi_loader/efi_helper.c  | 12 ++++++++++++
>  4 files changed, 59 insertions(+)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 8eda521693..b39e81ad30 100644
> --- a/boot/image-fdt.c
> +++ b/boot/image-fdt.c
> @@ -11,6 +11,7 @@
>  #include <command.h>
>  #include <fdt_support.h>
>  #include <fdtdec.h>
> +#include <efi_loader.h>
>  #include <env.h>
>  #include <errno.h>
>  #include <image.h>
> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
>         if (!ft_verify_fdt(blob))
>                 goto err;
>
> +       if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> +               fdt_ret = fdt_efi_pmem_setup(blob);
> +               if (fdt_ret) {
> +                       printf("ERROR: HTTP boot pmem fixup failed\n");
> +                       goto err;
> +               }
> +       }
> +
>         /* after here we are using a livetree */
>         if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) {
>                 struct event_ft_fixup fixup;
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index d450e304c6..031de18746 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
>  efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>                                     efi_guid_t *guid);
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:       Pointer to the devicetree
> + *
> + * Return:     0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt);
> +
>  /**
>   * efi_size_in_pages() - convert size in bytes to size in pages
>   *
> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>                                   void *load_options);
>  efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:       Pointer to the DT blob
> + * Return:     status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> +
>  /**
>   * struct efi_image_regions - A list of memory regions
>   *
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 16f75555f6..1d9246be61 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -41,6 +41,8 @@ struct uridp_context {
>         efi_handle_t mem_handle;
>  };
>
> +static struct uridp_context *uctx;
> +
>  const efi_guid_t efi_guid_bootmenu_auto_generated =
>                 EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
>
> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>
>         efi_free_pool(ctx->loaded_dp);
>         free(ctx);
> +       uctx = NULL;
>
>         return ret == EFI_SUCCESS ? ret2 : ret;
>  }
> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
>         EFI_EXIT(ret);
>  }
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:       Pointer to the DT blob
> + * Return:     status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> +{
> +       if (!uctx) {
> +               log_warning("No EFI HTTP boot context found\n");
> +               return EFI_SUCCESS;
> +       }
> +
> +       return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> +               EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +}
> +
>  /**
>   * try_load_from_uri_path() - Handle the URI device path
>   *
> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>         if (!ctx)
>                 return EFI_OUT_OF_RESOURCES;
>
> +       uctx = ctx;
>         s = env_get("loadaddr");
>         if (!s) {
>                 log_err("Error: loadaddr is not set\n");
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 00167bd2a1..33cd8b9a50 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
>         return 0;
>  }
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:       Pointer to the devicetree
> + *
> + * Return:     0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt)
> +{
> +       return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> +}
> +
>  static int u16_tohex(u16 c)
>  {
>         if (c >= '0' && c <= '9')
> --
> 2.34.1
>
Heinrich Schuchardt Oct. 28, 2024, 6:57 a.m. UTC | #2
On 10/25/24 13:14, Sughosh Ganu wrote:
> The EFI HTTP boot puts the iso installer image at some location in
> memory which needs to be reserved in the devicetree as persistent
> memory (pmem). Add helper functions which add this pmem node when the
> EFI_DT_FIXUP protocol's fixup callback is invoked.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   boot/image-fdt.c             |  9 +++++++++
>   include/efi_loader.h         | 17 +++++++++++++++++
>   lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
>   lib/efi_loader/efi_helper.c  | 12 ++++++++++++
>   4 files changed, 59 insertions(+)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 8eda521693..b39e81ad30 100644
> --- a/boot/image-fdt.c
> +++ b/boot/image-fdt.c
> @@ -11,6 +11,7 @@
>   #include <command.h>
>   #include <fdt_support.h>
>   #include <fdtdec.h>
> +#include <efi_loader.h>
>   #include <env.h>
>   #include <errno.h>
>   #include <image.h>
> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
>   	if (!ft_verify_fdt(blob))
>   		goto err;
>
> +	if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> +		fdt_ret = fdt_efi_pmem_setup(blob);

Having EFI_HTTP_BOOT is not the only way to load an image to memory. You
could have downloaded it via TFTP or taken it from a block device. So
this config check looks wrong. Why don't you check for the presence of
memory block devices here?

You need to check if the board is using ACPI, if yes, provide the ACPI
table.

Best regards

Heinrich

> +		if (fdt_ret) {
> +			printf("ERROR: HTTP boot pmem fixup failed\n");
> +			goto err;
> +		}
> +	}
> +
>   	/* after here we are using a livetree */
>   	if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) {
>   		struct event_ft_fixup fixup;
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index d450e304c6..031de18746 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
>   efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>   				    efi_guid_t *guid);
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:	Pointer to the devicetree
> + *
> + * Return:	0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt);
> +
>   /**
>    * efi_size_in_pages() - convert size in bytes to size in pages
>    *
> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>   				  void *load_options);
>   efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:	Pointer to the DT blob
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> +
>   /**
>    * struct efi_image_regions - A list of memory regions
>    *
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 16f75555f6..1d9246be61 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -41,6 +41,8 @@ struct uridp_context {
>   	efi_handle_t mem_handle;
>   };
>
> +static struct uridp_context *uctx;
> +
>   const efi_guid_t efi_guid_bootmenu_auto_generated =
>   		EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
>
> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>
>   	efi_free_pool(ctx->loaded_dp);
>   	free(ctx);
> +	uctx = NULL;
>
>   	return ret == EFI_SUCCESS ? ret2 : ret;
>   }
> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
>   	EFI_EXIT(ret);
>   }
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:	Pointer to the DT blob
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> +{
> +	if (!uctx) {
> +		log_warning("No EFI HTTP boot context found\n");
> +		return EFI_SUCCESS;
> +	}
> +
> +	return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> +		EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +}
> +
>   /**
>    * try_load_from_uri_path() - Handle the URI device path
>    *
> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>   	if (!ctx)
>   		return EFI_OUT_OF_RESOURCES;
>
> +	uctx = ctx;
>   	s = env_get("loadaddr");
>   	if (!s) {
>   		log_err("Error: loadaddr is not set\n");
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 00167bd2a1..33cd8b9a50 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
>   	return 0;
>   }
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:	Pointer to the devicetree
> + *
> + * Return:	0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt)
> +{
> +	return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> +}
> +
>   static int u16_tohex(u16 c)
>   {
>   	if (c >= '0' && c <= '9')
Ilias Apalodimas Oct. 29, 2024, 7:33 a.m. UTC | #3
Hi Heinrich

On Mon, 28 Oct 2024 at 08:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/25/24 13:14, Sughosh Ganu wrote:
> > The EFI HTTP boot puts the iso installer image at some location in
> > memory which needs to be reserved in the devicetree as persistent
> > memory (pmem). Add helper functions which add this pmem node when the
> > EFI_DT_FIXUP protocol's fixup callback is invoked.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   boot/image-fdt.c             |  9 +++++++++
> >   include/efi_loader.h         | 17 +++++++++++++++++
> >   lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
> >   lib/efi_loader/efi_helper.c  | 12 ++++++++++++
> >   4 files changed, 59 insertions(+)
> >
> > diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> > index 8eda521693..b39e81ad30 100644
> > --- a/boot/image-fdt.c
> > +++ b/boot/image-fdt.c
> > @@ -11,6 +11,7 @@
> >   #include <command.h>
> >   #include <fdt_support.h>
> >   #include <fdtdec.h>
> > +#include <efi_loader.h>
> >   #include <env.h>
> >   #include <errno.h>
> >   #include <image.h>
> > @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
> >       if (!ft_verify_fdt(blob))
> >               goto err;
> >
> > +     if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> > +             fdt_ret = fdt_efi_pmem_setup(blob);
>
> Having EFI_HTTP_BOOT is not the only way to load an image to memory. You
> could have downloaded it via TFTP or taken it from a block device. So
> this config check looks wrong. Why don't you check for the presence of
> memory block devices here?

If it's an existing block device (e.g a USB), there's no need to
preserve the .iso. The installer will scan the disks and find it
again.
TFTP might make sense, but this only applies to booting EFI images, so
scanning the memory block devices is not enough.

Sughosh can you take a look and see if scanning memory blocks is doable?

>
> You need to check if the board is using ACPI, if yes, provide the ACPI
> table.

The ACPI table is out of scope on these patches. We need to add a
check and only do the fixup if we are booting with a DT, but we dont
currently plan on fixing NFIT ACPI tables.

Thanks
/Ilias

> Best regards
>
> Heinrich
>
> > +             if (fdt_ret) {
> > +                     printf("ERROR: HTTP boot pmem fixup failed\n");
> > +                     goto err;
> > +             }
> > +     }
> > +
> >       /* after here we are using a livetree */
> >       if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) {
> >               struct event_ft_fixup fixup;
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index d450e304c6..031de18746 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
> >   efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
> >                                   efi_guid_t *guid);
> >
> > +/**
> > + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> > + *
> > + * @fdt:     Pointer to the devicetree
> > + *
> > + * Return:   0 on success, negative on failure
> > + */
> > +int fdt_efi_pmem_setup(void *fdt);
> > +
> >   /**
> >    * efi_size_in_pages() - convert size in bytes to size in pages
> >    *
> > @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> >                                 void *load_options);
> >   efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> >
> > +/**
> > + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> > + *
> > + * @fdt:     Pointer to the DT blob
> > + * Return:   status code
> > + */
> > +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> > +
> >   /**
> >    * struct efi_image_regions - A list of memory regions
> >    *
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 16f75555f6..1d9246be61 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -41,6 +41,8 @@ struct uridp_context {
> >       efi_handle_t mem_handle;
> >   };
> >
> > +static struct uridp_context *uctx;
> > +
> >   const efi_guid_t efi_guid_bootmenu_auto_generated =
> >               EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
> >
> > @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
> >
> >       efi_free_pool(ctx->loaded_dp);
> >       free(ctx);
> > +     uctx = NULL;
> >
> >       return ret == EFI_SUCCESS ? ret2 : ret;
> >   }
> > @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
> >       EFI_EXIT(ret);
> >   }
> >
> > +/**
> > + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> > + *
> > + * @fdt:     Pointer to the DT blob
> > + * Return:   status code
> > + */
> > +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> > +{
> > +     if (!uctx) {
> > +             log_warning("No EFI HTTP boot context found\n");
> > +             return EFI_SUCCESS;
> > +     }
> > +
> > +     return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> > +             EFI_SUCCESS : EFI_INVALID_PARAMETER;
> > +}
> > +
> >   /**
> >    * try_load_from_uri_path() - Handle the URI device path
> >    *
> > @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> >       if (!ctx)
> >               return EFI_OUT_OF_RESOURCES;
> >
> > +     uctx = ctx;
> >       s = env_get("loadaddr");
> >       if (!s) {
> >               log_err("Error: loadaddr is not set\n");
> > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> > index 00167bd2a1..33cd8b9a50 100644
> > --- a/lib/efi_loader/efi_helper.c
> > +++ b/lib/efi_loader/efi_helper.c
> > @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
> >       return 0;
> >   }
> >
> > +/**
> > + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> > + *
> > + * @fdt:     Pointer to the devicetree
> > + *
> > + * Return:   0 on success, negative on failure
> > + */
> > +int fdt_efi_pmem_setup(void *fdt)
> > +{
> > +     return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> > +}
> > +
> >   static int u16_tohex(u16 c)
> >   {
> >       if (c >= '0' && c <= '9')
>
Heinrich Schuchardt Nov. 11, 2024, 8:03 a.m. UTC | #4
On 10/25/24 13:14, Sughosh Ganu wrote:
> The EFI HTTP boot puts the iso installer image at some location in
> memory which needs to be reserved in the devicetree as persistent
> memory (pmem). Add helper functions which add this pmem node when the
> EFI_DT_FIXUP protocol's fixup callback is invoked.
>
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
>   boot/image-fdt.c             |  9 +++++++++
>   include/efi_loader.h         | 17 +++++++++++++++++
>   lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
>   lib/efi_loader/efi_helper.c  | 12 ++++++++++++
>   4 files changed, 59 insertions(+)
>
> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> index 8eda521693..b39e81ad30 100644
> --- a/boot/image-fdt.c
> +++ b/boot/image-fdt.c
> @@ -11,6 +11,7 @@
>   #include <command.h>
>   #include <fdt_support.h>
>   #include <fdtdec.h>
> +#include <efi_loader.h>
>   #include <env.h>
>   #include <errno.h>
>   #include <image.h>
> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
>   	if (!ft_verify_fdt(blob))
>   		goto err;
>
> +	if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> +		fdt_ret = fdt_efi_pmem_setup(blob);

I can see no reason why pmem setup should depend on HTTP boot.

It should be possible to pass a memory block device to the kernel no
matter how it was created.

Best regards

Heinrich

> +		if (fdt_ret) {
> +			printf("ERROR: HTTP boot pmem fixup failed\n");
> +			goto err;
> +		}
> +	}
> +
>   	/* after here we are using a livetree */
>   	if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) {
>   		struct event_ft_fixup fixup;
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index d450e304c6..031de18746 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
>   efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>   				    efi_guid_t *guid);
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:	Pointer to the devicetree
> + *
> + * Return:	0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt);
> +
>   /**
>    * efi_size_in_pages() - convert size in bytes to size in pages
>    *
> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>   				  void *load_options);
>   efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:	Pointer to the DT blob
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> +
>   /**
>    * struct efi_image_regions - A list of memory regions
>    *
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 16f75555f6..1d9246be61 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -41,6 +41,8 @@ struct uridp_context {
>   	efi_handle_t mem_handle;
>   };
>
> +static struct uridp_context *uctx;
> +
>   const efi_guid_t efi_guid_bootmenu_auto_generated =
>   		EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
>
> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>
>   	efi_free_pool(ctx->loaded_dp);
>   	free(ctx);
> +	uctx = NULL;
>
>   	return ret == EFI_SUCCESS ? ret2 : ret;
>   }
> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
>   	EFI_EXIT(ret);
>   }
>
> +/**
> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> + *
> + * @fdt:	Pointer to the DT blob
> + * Return:	status code
> + */
> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> +{
> +	if (!uctx) {
> +		log_warning("No EFI HTTP boot context found\n");
> +		return EFI_SUCCESS;
> +	}
> +
> +	return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> +		EFI_SUCCESS : EFI_INVALID_PARAMETER;
> +}
> +
>   /**
>    * try_load_from_uri_path() - Handle the URI device path
>    *
> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>   	if (!ctx)
>   		return EFI_OUT_OF_RESOURCES;
>
> +	uctx = ctx;
>   	s = env_get("loadaddr");
>   	if (!s) {
>   		log_err("Error: loadaddr is not set\n");
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 00167bd2a1..33cd8b9a50 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
>   	return 0;
>   }
>
> +/**
> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> + *
> + * @fdt:	Pointer to the devicetree
> + *
> + * Return:	0 on success, negative on failure
> + */
> +int fdt_efi_pmem_setup(void *fdt)
> +{
> +	return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> +}
> +
>   static int u16_tohex(u16 c)
>   {
>   	if (c >= '0' && c <= '9')
Ilias Apalodimas Nov. 11, 2024, 10:45 a.m. UTC | #5
Hi Heinrich

On Mon, 11 Nov 2024 at 10:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/25/24 13:14, Sughosh Ganu wrote:
> > The EFI HTTP boot puts the iso installer image at some location in
> > memory which needs to be reserved in the devicetree as persistent
> > memory (pmem). Add helper functions which add this pmem node when the
> > EFI_DT_FIXUP protocol's fixup callback is invoked.
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >   boot/image-fdt.c             |  9 +++++++++
> >   include/efi_loader.h         | 17 +++++++++++++++++
> >   lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
> >   lib/efi_loader/efi_helper.c  | 12 ++++++++++++
> >   4 files changed, 59 insertions(+)
> >
> > diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> > index 8eda521693..b39e81ad30 100644
> > --- a/boot/image-fdt.c
> > +++ b/boot/image-fdt.c
> > @@ -11,6 +11,7 @@
> >   #include <command.h>
> >   #include <fdt_support.h>
> >   #include <fdtdec.h>
> > +#include <efi_loader.h>
> >   #include <env.h>
> >   #include <errno.h>
> >   #include <image.h>
> > @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
> >       if (!ft_verify_fdt(blob))
> >               goto err;
> >
> > +     if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> > +             fdt_ret = fdt_efi_pmem_setup(blob);
>
> I can see no reason why pmem setup should depend on HTTP boot.

The reason is that we *know* we want to preserve image on HTTP
installers. but we should only add it if that image is booted, not
unconditionally if EFI_HTTP is enabled

>
> It should be possible to pass a memory block device to the kernel no
> matter how it was created.

Yes, we can. But how do we know we need to setup a pmem node? In this
specific case, we know http installers need the image.
If we can find similar rules (or perhaps a command line option), we
can preserve it for all images

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> > +             if (fdt_ret) {
> > +                     printf("ERROR: HTTP boot pmem fixup failed\n");
> > +                     goto err;
> > +             }
> > +     }
> > +
> >       /* after here we are using a livetree */
> >       if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) {
> >               struct event_ft_fixup fixup;
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index d450e304c6..031de18746 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
> >   efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
> >                                   efi_guid_t *guid);
> >
> > +/**
> > + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> > + *
> > + * @fdt:     Pointer to the devicetree
> > + *
> > + * Return:   0 on success, negative on failure
> > + */
> > +int fdt_efi_pmem_setup(void *fdt);
> > +
> >   /**
> >    * efi_size_in_pages() - convert size in bytes to size in pages
> >    *
> > @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> >                                 void *load_options);
> >   efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> >
> > +/**
> > + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> > + *
> > + * @fdt:     Pointer to the DT blob
> > + * Return:   status code
> > + */
> > +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> > +
> >   /**
> >    * struct efi_image_regions - A list of memory regions
> >    *
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 16f75555f6..1d9246be61 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -41,6 +41,8 @@ struct uridp_context {
> >       efi_handle_t mem_handle;
> >   };
> >
> > +static struct uridp_context *uctx;
> > +
> >   const efi_guid_t efi_guid_bootmenu_auto_generated =
> >               EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
> >
> > @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
> >
> >       efi_free_pool(ctx->loaded_dp);
> >       free(ctx);
> > +     uctx = NULL;
> >
> >       return ret == EFI_SUCCESS ? ret2 : ret;
> >   }
> > @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
> >       EFI_EXIT(ret);
> >   }
> >
> > +/**
> > + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> > + *
> > + * @fdt:     Pointer to the DT blob
> > + * Return:   status code
> > + */
> > +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> > +{
> > +     if (!uctx) {
> > +             log_warning("No EFI HTTP boot context found\n");
> > +             return EFI_SUCCESS;
> > +     }
> > +
> > +     return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> > +             EFI_SUCCESS : EFI_INVALID_PARAMETER;
> > +}
> > +
> >   /**
> >    * try_load_from_uri_path() - Handle the URI device path
> >    *
> > @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> >       if (!ctx)
> >               return EFI_OUT_OF_RESOURCES;
> >
> > +     uctx = ctx;
> >       s = env_get("loadaddr");
> >       if (!s) {
> >               log_err("Error: loadaddr is not set\n");
> > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> > index 00167bd2a1..33cd8b9a50 100644
> > --- a/lib/efi_loader/efi_helper.c
> > +++ b/lib/efi_loader/efi_helper.c
> > @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
> >       return 0;
> >   }
> >
> > +/**
> > + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> > + *
> > + * @fdt:     Pointer to the devicetree
> > + *
> > + * Return:   0 on success, negative on failure
> > + */
> > +int fdt_efi_pmem_setup(void *fdt)
> > +{
> > +     return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> > +}
> > +
> >   static int u16_tohex(u16 c)
> >   {
> >       if (c >= '0' && c <= '9')
>
Heinrich Schuchardt Nov. 11, 2024, 11:33 a.m. UTC | #6
On 11/11/24 11:45, Ilias Apalodimas wrote:
> Hi Heinrich
>
> On Mon, 11 Nov 2024 at 10:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 10/25/24 13:14, Sughosh Ganuefi_bootmgr_pmem_setup wrote:
>>> The EFI HTTP boot puts the iso installer image at some location in
>>> memory which needs to be reserved in the devicetree as persistent
>>> memory (pmem). Add helper functions which add this pmem node when the
>>> EFI_DT_FIXUP protocol's fixup callback is invoked.
>>>
>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>> ---
>>>    boot/image-fdt.c             |  9 +++++++++
>>>    include/efi_loader.h         | 17 +++++++++++++++++
>>>    lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
>>>    lib/efi_loader/efi_helper.c  | 12 ++++++++++++
>>>    4 files changed, 59 insertions(+)
>>>
>>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
>>> index 8eda521693..b39e81ad30 100644
>>> --- a/boot/image-fdt.c
>>> +++ b/boot/image-fdt.c
>>> @@ -11,6 +11,7 @@
>>>    #include <command.h>
>>>    #include <fdt_support.h>
>>>    #include <fdtdec.h>
>>> +#include <efi_loader.h>
>>>    #include <env.h>
>>>    #include <errno.h>
>>>    #include <image.h>
>>> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
>>>        if (!ft_verify_fdt(blob))
>>>                goto err;
>>>
>>> +     if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
>>> +             fdt_ret = fdt_efi_pmem_setup(blob);
>>
>> I can see no reason why pmem setup should depend on HTTP boot.
>
> The reason is that we *know* we want to preserve image on HTTP
> installers. but we should only add it if that image is booted, not
> unconditionally if EFI_HTTP is enabled
>
>>
>> It should be possible to pass a memory block device to the kernel no
>> matter how it was created.
>
> Yes, we can. But how do we know we need to setup a pmem node? In this
> specific case, we know http installers need the image.
> If we can find similar rules (or perhaps a command line option), we
> can preserve it for all images

Simon is working on patches to track loaded images. I guess there we
would need to add the detection of image types including bare kernels,
EFI binaries, and ISOs.

>
> Thanks
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>> +             if (fdt_ret) {
>>> +                     printf("ERROR: HTTP boot pmem fixup failed\n");

Please, use log_err() and remove "ERROR: ".

>>> +                     goto err;
>>> +             }
>>> +     }
>>> +
>>>        /* after here we are using a livetree */
>>>        if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) {
>>>                struct event_ft_fixup fixup;
>>> diff --git a/include/efi_loader.h b/include/efi_loader.h
>>> index d450e304c6..031de18746 100644
>>> --- a/include/efi_loader.h
>>> +++ b/include/efi_loader.h
>>> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
>>>    efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
>>>                                    efi_guid_t *guid);
>>>
>>> +/**
>>> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
>>> + *
>>> + * @fdt:     Pointer to the devicetree
>>> + *
>>> + * Return:   0 on success, negative on failure
>>> + */
>>> +int fdt_efi_pmem_setup(void *fdt);
>>> +
>>>    /**
>>>     * efi_size_in_pages() - convert size in bytes to size in pages
>>>     *
>>> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
>>>                                  void *load_options);
>>>    efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
>>>
>>> +/**
>>> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
>>> + *
>>> + * @fdt:     Pointer to the DT blob
>>> + * Return:   status code
>>> + */
>>> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
>>> +
>>>    /**
>>>     * struct efi_image_regions - A list of memory regions
>>>     *
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index 16f75555f6..1d9246be61 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -41,6 +41,8 @@ struct uridp_context {
>>>        efi_handle_t mem_handle;
>>>    };
>>>
>>> +static struct uridp_context *uctx;
>>> +
>>>    const efi_guid_t efi_guid_bootmenu_auto_generated =
>>>                EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
>>>
>>> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
>>>
>>>        efi_free_pool(ctx->loaded_dp);
>>>        free(ctx);
>>> +     uctx = NULL;
>>>
>>>        return ret == EFI_SUCCESS ? ret2 : ret;
>>>    }
>>> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
>>>        EFI_EXIT(ret);
>>>    }
>>>
>>> +/**
>>> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
>>> + *
>>> + * @fdt:     Pointer to the DT blob
>>> + * Return:   status code
>>> + */
>>> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
>>> +{
>>> +     if (!uctx) {
>>> +             log_warning("No EFI HTTP boot context found\n");

Are we writing a warning here when loading grubriscv64.efi from disk and
executing it?

Best regards

Heinrich

>>> +             return EFI_SUCCESS;
>>> +     }
>>> +
>>> +     return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
>>> +             EFI_SUCCESS : EFI_INVALID_PARAMETER;
>>> +}
>>> +
>>>    /**
>>>     * try_load_from_uri_path() - Handle the URI device path
>>>     *
>>> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>>>        if (!ctx)
>>>                return EFI_OUT_OF_RESOURCES;
>>>
>>> +     uctx = ctx;
>>>        s = env_get("loadaddr");
>>>        if (!s) {
>>>                log_err("Error: loadaddr is not set\n");
>>> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
>>> index 00167bd2a1..33cd8b9a50 100644
>>> --- a/lib/efi_loader/efi_helper.c
>>> +++ b/lib/efi_loader/efi_helper.c
>>> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
>>>        return 0;
>>>    }
>>>
>>> +/**
>>> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
>>> + *
>>> + * @fdt:     Pointer to the devicetree
>>> + *
>>> + * Return:   0 on success, negative on failure
>>> + */
>>> +int fdt_efi_pmem_setup(void *fdt)
>>> +{
>>> +     return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
>>> +}
>>> +
>>>    static int u16_tohex(u16 c)
>>>    {
>>>        if (c >= '0' && c <= '9')
>>
Ilias Apalodimas Nov. 11, 2024, 12:14 p.m. UTC | #7
On Mon, 11 Nov 2024 at 13:33, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11/11/24 11:45, Ilias Apalodimas wrote:
> > Hi Heinrich
> >
> > On Mon, 11 Nov 2024 at 10:08, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 10/25/24 13:14, Sughosh Ganuefi_bootmgr_pmem_setup wrote:
> >>> The EFI HTTP boot puts the iso installer image at some location in
> >>> memory which needs to be reserved in the devicetree as persistent
> >>> memory (pmem). Add helper functions which add this pmem node when the
> >>> EFI_DT_FIXUP protocol's fixup callback is invoked.
> >>>
> >>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> >>> ---
> >>>    boot/image-fdt.c             |  9 +++++++++
> >>>    include/efi_loader.h         | 17 +++++++++++++++++
> >>>    lib/efi_loader/efi_bootmgr.c | 21 +++++++++++++++++++++
> >>>    lib/efi_loader/efi_helper.c  | 12 ++++++++++++
> >>>    4 files changed, 59 insertions(+)
> >>>
> >>> diff --git a/boot/image-fdt.c b/boot/image-fdt.c
> >>> index 8eda521693..b39e81ad30 100644
> >>> --- a/boot/image-fdt.c
> >>> +++ b/boot/image-fdt.c
> >>> @@ -11,6 +11,7 @@
> >>>    #include <command.h>
> >>>    #include <fdt_support.h>
> >>>    #include <fdtdec.h>
> >>> +#include <efi_loader.h>
> >>>    #include <env.h>
> >>>    #include <errno.h>
> >>>    #include <image.h>
> >>> @@ -648,6 +649,14 @@ int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
> >>>        if (!ft_verify_fdt(blob))
> >>>                goto err;
> >>>
> >>> +     if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
> >>> +             fdt_ret = fdt_efi_pmem_setup(blob);
> >>
> >> I can see no reason why pmem setup should depend on HTTP boot.
> >
> > The reason is that we *know* we want to preserve image on HTTP
> > installers. but we should only add it if that image is booted, not
> > unconditionally if EFI_HTTP is enabled
> >
> >>
> >> It should be possible to pass a memory block device to the kernel no
> >> matter how it was created.
> >
> > Yes, we can. But how do we know we need to setup a pmem node? In this
> > specific case, we know http installers need the image.
> > If we can find similar rules (or perhaps a command line option), we
> > can preserve it for all images
>
> Simon is working on patches to track loaded images. I guess there we
> would need to add the detection of image types including bare kernels,
> EFI binaries, and ISOs.
>

Yes that sounds like a good idea.

> >
> > Thanks
> > /Ilias
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> +             if (fdt_ret) {
> >>> +                     printf("ERROR: HTTP boot pmem fixup failed\n");
>
> Please, use log_err() and remove "ERROR: ".
>
> >>> +                     goto err;
> >>> +             }
> >>> +     }
> >>> +
> >>>        /* after here we are using a livetree */
> >>>        if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) {
> >>>                struct event_ft_fixup fixup;
> >>> diff --git a/include/efi_loader.h b/include/efi_loader.h
> >>> index d450e304c6..031de18746 100644
> >>> --- a/include/efi_loader.h
> >>> +++ b/include/efi_loader.h
> >>> @@ -748,6 +748,15 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index);
> >>>    efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
> >>>                                    efi_guid_t *guid);
> >>>
> >>> +/**
> >>> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> >>> + *
> >>> + * @fdt:     Pointer to the devicetree
> >>> + *
> >>> + * Return:   0 on success, negative on failure
> >>> + */
> >>> +int fdt_efi_pmem_setup(void *fdt);
> >>> +
> >>>    /**
> >>>     * efi_size_in_pages() - convert size in bytes to size in pages
> >>>     *
> >>> @@ -964,6 +973,14 @@ efi_status_t efi_set_load_options(efi_handle_t handle,
> >>>                                  void *load_options);
> >>>    efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
> >>>
> >>> +/**
> >>> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> >>> + *
> >>> + * @fdt:     Pointer to the DT blob
> >>> + * Return:   status code
> >>> + */
> >>> +efi_status_t efi_bootmgr_pmem_setup(void *fdt);
> >>> +
> >>>    /**
> >>>     * struct efi_image_regions - A list of memory regions
> >>>     *
> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>> index 16f75555f6..1d9246be61 100644
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -41,6 +41,8 @@ struct uridp_context {
> >>>        efi_handle_t mem_handle;
> >>>    };
> >>>
> >>> +static struct uridp_context *uctx;
> >>> +
> >>>    const efi_guid_t efi_guid_bootmenu_auto_generated =
> >>>                EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
> >>>
> >>> @@ -423,6 +425,7 @@ efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
> >>>
> >>>        efi_free_pool(ctx->loaded_dp);
> >>>        free(ctx);
> >>> +     uctx = NULL;
> >>>
> >>>        return ret == EFI_SUCCESS ? ret2 : ret;
> >>>    }
> >>> @@ -443,6 +446,23 @@ static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
> >>>        EFI_EXIT(ret);
> >>>    }
> >>>
> >>> +/**
> >>> + * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
> >>> + *
> >>> + * @fdt:     Pointer to the DT blob
> >>> + * Return:   status code
> >>> + */
> >>> +efi_status_t efi_bootmgr_pmem_setup(void *fdt)
> >>> +{
> >>> +     if (!uctx) {
> >>> +             log_warning("No EFI HTTP boot context found\n");
>
> Are we writing a warning here when loading grubriscv64.efi from disk and
> executing it?

Probably, that links to my previous comment. This should only execute
when we *know* we boot an EFI image via HTTP, not for any load.
Since we only setup that context for EFI images, simply removing the
print works for now. But as you mentioned we should plug this better
in the future once Simon series land

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> >>> +             return EFI_SUCCESS;
> >>> +     }
> >>> +
> >>> +     return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
> >>> +             EFI_SUCCESS : EFI_INVALID_PARAMETER;
> >>> +}
> >>> +
> >>>    /**
> >>>     * try_load_from_uri_path() - Handle the URI device path
> >>>     *
> >>> @@ -472,6 +492,7 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> >>>        if (!ctx)
> >>>                return EFI_OUT_OF_RESOURCES;
> >>>
> >>> +     uctx = ctx;
> >>>        s = env_get("loadaddr");
> >>>        if (!s) {
> >>>                log_err("Error: loadaddr is not set\n");
> >>> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> >>> index 00167bd2a1..33cd8b9a50 100644
> >>> --- a/lib/efi_loader/efi_helper.c
> >>> +++ b/lib/efi_loader/efi_helper.c
> >>> @@ -242,6 +242,18 @@ int efi_unlink_dev(efi_handle_t handle)
> >>>        return 0;
> >>>    }
> >>>
> >>> +/**
> >>> + * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
> >>> + *
> >>> + * @fdt:     Pointer to the devicetree
> >>> + *
> >>> + * Return:   0 on success, negative on failure
> >>> + */
> >>> +int fdt_efi_pmem_setup(void *fdt)
> >>> +{
> >>> +     return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
> >>> +}
> >>> +
> >>>    static int u16_tohex(u16 c)
> >>>    {
> >>>        if (c >= '0' && c <= '9')
> >>
>
diff mbox series

Patch

diff --git a/boot/image-fdt.c b/boot/image-fdt.c
index 8eda521693..b39e81ad30 100644
--- a/boot/image-fdt.c
+++ b/boot/image-fdt.c
@@ -11,6 +11,7 @@ 
 #include <command.h>
 #include <fdt_support.h>
 #include <fdtdec.h>
+#include <efi_loader.h>
 #include <env.h>
 #include <errno.h>
 #include <image.h>
@@ -648,6 +649,14 @@  int image_setup_libfdt(struct bootm_headers *images, void *blob, bool lmb)
 	if (!ft_verify_fdt(blob))
 		goto err;
 
+	if (CONFIG_IS_ENABLED(EFI_HTTP_BOOT)) {
+		fdt_ret = fdt_efi_pmem_setup(blob);
+		if (fdt_ret) {
+			printf("ERROR: HTTP boot pmem fixup failed\n");
+			goto err;
+		}
+	}
+
 	/* after here we are using a livetree */
 	if (!of_live_active() && CONFIG_IS_ENABLED(EVENT)) {
 		struct event_ft_fixup fixup;
diff --git a/include/efi_loader.h b/include/efi_loader.h
index d450e304c6..031de18746 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -748,6 +748,15 @@  bool efi_varname_is_load_option(u16 *var_name16, int *index);
 efi_status_t efi_next_variable_name(efi_uintn_t *size, u16 **buf,
 				    efi_guid_t *guid);
 
+/**
+ * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
+ *
+ * @fdt:	Pointer to the devicetree
+ *
+ * Return:	0 on success, negative on failure
+ */
+int fdt_efi_pmem_setup(void *fdt);
+
 /**
  * efi_size_in_pages() - convert size in bytes to size in pages
  *
@@ -964,6 +973,14 @@  efi_status_t efi_set_load_options(efi_handle_t handle,
 				  void *load_options);
 efi_status_t efi_bootmgr_load(efi_handle_t *handle, void **load_options);
 
+/**
+ * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
+ *
+ * @fdt:	Pointer to the DT blob
+ * Return:	status code
+ */
+efi_status_t efi_bootmgr_pmem_setup(void *fdt);
+
 /**
  * struct efi_image_regions - A list of memory regions
  *
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 16f75555f6..1d9246be61 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -41,6 +41,8 @@  struct uridp_context {
 	efi_handle_t mem_handle;
 };
 
+static struct uridp_context *uctx;
+
 const efi_guid_t efi_guid_bootmenu_auto_generated =
 		EFICONFIG_AUTO_GENERATED_ENTRY_GUID;
 
@@ -423,6 +425,7 @@  efi_status_t efi_bootmgr_release_uridp(struct uridp_context *ctx)
 
 	efi_free_pool(ctx->loaded_dp);
 	free(ctx);
+	uctx = NULL;
 
 	return ret == EFI_SUCCESS ? ret2 : ret;
 }
@@ -443,6 +446,23 @@  static void EFIAPI efi_bootmgr_http_return(struct efi_event *event,
 	EFI_EXIT(ret);
 }
 
+/**
+ * efi_bootmgr_pmem_setup() - Put a pmem node for UEFI HTTP installers
+ *
+ * @fdt:	Pointer to the DT blob
+ * Return:	status code
+ */
+efi_status_t efi_bootmgr_pmem_setup(void *fdt)
+{
+	if (!uctx) {
+		log_warning("No EFI HTTP boot context found\n");
+		return EFI_SUCCESS;
+	}
+
+	return !fdt_fixup_pmem_region(fdt, uctx->image_addr, uctx->image_size) ?
+		EFI_SUCCESS : EFI_INVALID_PARAMETER;
+}
+
 /**
  * try_load_from_uri_path() - Handle the URI device path
  *
@@ -472,6 +492,7 @@  static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
 	if (!ctx)
 		return EFI_OUT_OF_RESOURCES;
 
+	uctx = ctx;
 	s = env_get("loadaddr");
 	if (!s) {
 		log_err("Error: loadaddr is not set\n");
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index 00167bd2a1..33cd8b9a50 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -242,6 +242,18 @@  int efi_unlink_dev(efi_handle_t handle)
 	return 0;
 }
 
+/**
+ * fdt_efi_pmem_setup() - Setup the pmem node in the devicetree
+ *
+ * @fdt:	Pointer to the devicetree
+ *
+ * Return:	0 on success, negative on failure
+ */
+int fdt_efi_pmem_setup(void *fdt)
+{
+	return efi_bootmgr_pmem_setup(fdt) == EFI_SUCCESS ? 0 : -1;
+}
+
 static int u16_tohex(u16 c)
 {
 	if (c >= '0' && c <= '9')