* Re: [PATCH 2/2] efi_loader: support boot from URI device path
2023-08-23 8:37 ` [PATCH 2/2] efi_loader: support boot from URI " Masahisa Kojima
@ 2023-08-23 23:57 ` Simon Glass
2023-08-25 6:31 ` Masahisa Kojima
2023-08-24 2:24 ` AKASHI Takahiro
2023-08-24 6:58 ` Heinrich Schuchardt
2 siblings, 1 reply; 16+ messages in thread
From: Simon Glass @ 2023-08-23 23:57 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas
Hi Masahisa,
On Wed, 23 Aug 2023 at 02:38, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> This supports to boot from the URI device path.
> When user selects the URI device path, bootmgr downloads
> the file using wget into the address specified by loadaddr
> env variable.
> If the file is .iso or .img file, mount the image with blkmap
> then try to boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> If the file is .efi file, load and start the downloaded file.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> lib/efi_loader/efi_bootmgr.c | 213 +++++++++++++++++++++++++++++++++++
> 1 file changed, 213 insertions(+)
>
Much of this code should be factored out into a help which can be
called from other code. See comments below.
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a40762c74c..8b20f486f2 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -7,10 +7,14 @@
>
> #define LOG_CATEGORY LOGC_EFI
>
> +#include <blk.h>
> +#include <blkmap.h>
> #include <common.h>
> #include <charset.h>
> +#include <dm.h>
> #include <log.h>
> #include <malloc.h>
> +#include <net.h>
> #include <efi_default_filename.h>
> #include <efi_loader.h>
> #include <efi_variable.h>
> @@ -168,6 +172,209 @@ out:
> return ret;
> }
>
> +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> +/**
> + * mount_image() - mount the image
> + *
> + * @lo_label label of load option
> + * @file_size file size
> + * @handle: pointer to handle for newly installed image
> + * Return: status code
> + */
> +static efi_status_t mount_image(u16 *lo_label, int file_size,
> + efi_handle_t *handle)
> +{
> + int err;
> + efi_status_t ret;
> + char *label = NULL, *p;
> + lbaint_t blknum;
> + struct udevice *bm_dev;
> + efi_handle_t bm_handle;
> + struct udevice *blk, *partition;
> + struct efi_handler *handler;
> + struct efi_device_path *file_path;
> + struct efi_device_path *device_path;
> +
> + label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> + if (!label)
> + return EFI_OUT_OF_RESOURCES;
> +
From here:
> + p = label;
> + utf16_utf8_strcpy(&p, lo_label);
> + err = blkmap_create(label, NULL);
> + if (err) {
> + log_err("failed to create blkmap\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + bm_dev = blkmap_from_label(label);
> + if (!bm_dev) {
> + log_err("\"%s\" is not the name of any known blkmap\n", label);
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + blknum = file_size / 512; /* TODO: don't use literal value. */
> + err = blkmap_map_pmem(bm_dev, 0, blknum, image_load_addr);
> + if (err) {
> + log_err("Unable to map %#llx at block %d : %d\n",
> + (unsigned long long)image_load_addr, 0, err);
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + log_info("Block %d+0x" LBAF " mapped to %#llx\n", 0, blknum,
> + (unsigned long long)image_load_addr);
> +
> + /* TODO: without calling this, partition devices are not binded. */
> + blk_list_part(UCLASS_BLKMAP);
> +
> + /*
> + * Search the partition having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> + */
> + device_foreach_child(blk, bm_dev)
> + {
check code style
> + device_foreach_child(partition, blk)
> + {
to here.
Perhaps have partition_first() and partition_next() iterators?
From here is EFI code:
> + if (dev_tag_get_ptr(partition, DM_TAG_EFI,
> + (void **)&bm_handle)) {
> + log_warning("DM_TAG_EFI not found\n");
> + continue;
> + }
> +
> + ret = efi_search_protocol(
> + bm_handle,
> + &efi_simple_file_system_protocol_guid,
> + &handler);
> + if (ret != EFI_SUCCESS)
> + continue;
> +
> + ret = efi_search_protocol(
> + bm_handle, &efi_guid_device_path, &handler);
> + if (ret != EFI_SUCCESS)
> + continue;
> +
> + ret = efi_protocol_open(handler, (void **)&device_path,
> + efi_root, NULL,
> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> + if (ret != EFI_SUCCESS)
> + continue;
> +
> + file_path = expand_media_path(device_path);
> + ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> + NULL, 0, handle));
> + efi_free_pool(file_path);
> + if (ret == EFI_SUCCESS)
> + goto out;
> + }
> + }
> +
> +out:
> + efi_free_pool(label);
> +
> + return ret;
> +}
> +
> +/**
> + * try_load_from_uri_path() - Handle the URI device path
> + *
> + * @uridp: uri device path
> + * @lo_label label of load option
> + * @handle: pointer to handle for newly installed image
> + * Return: status code
> + */
> +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> + u16 *lo_label,
> + efi_handle_t *handle)
> +{
> + efi_status_t ret;
> + int file_size, file_name_len;
> + char *s, *host_name, *file_name, *str_copy;
> +
From here should be normal caode:
> + /*
> + * Download file using wget.
> + *
> + * URI device path content is like http://www.example.com/sample/test.iso.
> + * U-Boot wget takes the target uri in this format.
> + * "<http server ip>:<file path>" e.g.) 192.168.1.1:/sample/test.iso
> + * Need to resolve the http server ip address before starting wget.
> + */
> +
> + /* only support "http://" */
> + if (strncmp(uridp->uri, "http://", 7)) {
> + log_err("Error: uri must start with http://\n");
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + str_copy = strdup(uridp->uri);
> + if (!str_copy)
> + return EFI_OUT_OF_RESOURCES;
> + s = str_copy + strlen("http://");
> + host_name = strsep(&s, "/");
> + if (!s) {
> + log_err("Error: invalied uri, no file path\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + file_name = s;
> + net_dns_resolve = host_name;
> + net_dns_env_var = "httpserverip";
> + if (net_loop(DNS) < 0) {
> + log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + s = env_get("httpserverip");
> + if (!s) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + /*
> + * WGET requires that "net_boot_file_name" and "image_load_addr" global
> + * variables are properly set in advance.
> + */
> + strlcpy(net_boot_file_name, s, 1024);
> + strlcat(net_boot_file_name, ":/", 1024); /* append '/' which is removed by strsep() */
> + strlcat(net_boot_file_name, file_name, 1024);
> + s = env_get("loadaddr");
> + if (!s) {
> + log_err("Error: loadaddr is not set\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + image_load_addr = hextoul(s, NULL);
> +
> + file_size = net_loop(WGET);
> + if (file_size < 0) {
> + log_err("Error: downloading file failed\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + /*
> + * Identify file type by file extension.
> + * If the file extension is ".iso" or ".img", mount it and boot with default file.
> + * If the file is ".efi", load and start the downloaded file.
> + */
> + file_name_len = strlen(net_boot_file_name);
to here.
You could package that up into a function which returns the filename.
Then the EFI code is this:
> + if (!strncmp(&net_boot_file_name[file_name_len - 4], ".iso", 4) ||
> + !strncmp(&net_boot_file_name[file_name_len - 4], ".img", 4)) {
> + ret = mount_image(lo_label, file_size, handle);
> + } else if (!strncmp(&net_boot_file_name[file_name_len - 4], ".efi", 4)) {
> + ret = efi_run_image((void *)image_load_addr, file_size);
> + } else {
> + log_err("Error: file type is not supported\n");
> + ret = EFI_INVALID_PARAMETER;
> + }
> +
> +out:
> + free(str_copy);
> +
> + return ret;
> +}
> +#endif
> +
> /**
> * try_load_entry() - try to load image for boot option
> *
> @@ -211,6 +418,12 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
> /* file_path doesn't contain a device path */
> ret = try_load_from_short_path(lo.file_path, handle);
> +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> + } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
P> + ret = try_load_from_uri_path(
> + (struct efi_device_path_uri *)lo.file_path,
> + lo.label, handle);
> +#endif
> } else {
> file_path = expand_media_path(lo.file_path);
> ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> --
> 2.34.1
>
Also please consider how to test this. With the changes I mention
above, you can fairly easily test the partition stuff (we have several
mmc images for testing).
Regards,
Simon
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] efi_loader: support boot from URI device path
2023-08-23 23:57 ` Simon Glass
@ 2023-08-25 6:31 ` Masahisa Kojima
0 siblings, 0 replies; 16+ messages in thread
From: Masahisa Kojima @ 2023-08-25 6:31 UTC (permalink / raw)
To: Simon Glass; +Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas
On Thu, 24 Aug 2023 at 08:59, Simon Glass <sjg@google.com> wrote:
>
> Hi Masahisa,
>
> On Wed, 23 Aug 2023 at 02:38, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > This supports to boot from the URI device path.
> > When user selects the URI device path, bootmgr downloads
> > the file using wget into the address specified by loadaddr
> > env variable.
> > If the file is .iso or .img file, mount the image with blkmap
> > then try to boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > If the file is .efi file, load and start the downloaded file.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > lib/efi_loader/efi_bootmgr.c | 213 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 213 insertions(+)
> >
>
> Much of this code should be factored out into a help which can be
> called from other code. See comments below.
>
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a40762c74c..8b20f486f2 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -7,10 +7,14 @@
> >
> > #define LOG_CATEGORY LOGC_EFI
> >
> > +#include <blk.h>
> > +#include <blkmap.h>
> > #include <common.h>
> > #include <charset.h>
> > +#include <dm.h>
> > #include <log.h>
> > #include <malloc.h>
> > +#include <net.h>
> > #include <efi_default_filename.h>
> > #include <efi_loader.h>
> > #include <efi_variable.h>
> > @@ -168,6 +172,209 @@ out:
> > return ret;
> > }
> >
> > +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> > +/**
> > + * mount_image() - mount the image
> > + *
> > + * @lo_label label of load option
> > + * @file_size file size
> > + * @handle: pointer to handle for newly installed image
> > + * Return: status code
> > + */
> > +static efi_status_t mount_image(u16 *lo_label, int file_size,
> > + efi_handle_t *handle)
> > +{
> > + int err;
> > + efi_status_t ret;
> > + char *label = NULL, *p;
> > + lbaint_t blknum;
> > + struct udevice *bm_dev;
> > + efi_handle_t bm_handle;
> > + struct udevice *blk, *partition;
> > + struct efi_handler *handler;
> > + struct efi_device_path *file_path;
> > + struct efi_device_path *device_path;
> > +
> > + label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> > + if (!label)
> > + return EFI_OUT_OF_RESOURCES;
> > +
>
> From here:
>
> > + p = label;
> > + utf16_utf8_strcpy(&p, lo_label);
> > + err = blkmap_create(label, NULL);
> > + if (err) {
> > + log_err("failed to create blkmap\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + bm_dev = blkmap_from_label(label);
> > + if (!bm_dev) {
> > + log_err("\"%s\" is not the name of any known blkmap\n", label);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + blknum = file_size / 512; /* TODO: don't use literal value. */
> > + err = blkmap_map_pmem(bm_dev, 0, blknum, image_load_addr);
> > + if (err) {
> > + log_err("Unable to map %#llx at block %d : %d\n",
> > + (unsigned long long)image_load_addr, 0, err);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + log_info("Block %d+0x" LBAF " mapped to %#llx\n", 0, blknum,
> > + (unsigned long long)image_load_addr);
> > +
> > + /* TODO: without calling this, partition devices are not binded. */
> > + blk_list_part(UCLASS_BLKMAP);
> > +
> > + /*
> > + * Search the partition having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> > + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > + */
> > + device_foreach_child(blk, bm_dev)
> > + {
>
> check code style
OK.
>
> > + device_foreach_child(partition, blk)
> > + {
>
> to here.
OK, I will factor out the non-EFI code for reuse.
>
> Perhaps have partition_first() and partition_next() iterators?
>
> From here is EFI code:
>
> > + if (dev_tag_get_ptr(partition, DM_TAG_EFI,
> > + (void **)&bm_handle)) {
> > + log_warning("DM_TAG_EFI not found\n");
> > + continue;
> > + }
> > +
> > + ret = efi_search_protocol(
> > + bm_handle,
> > + &efi_simple_file_system_protocol_guid,
> > + &handler);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + ret = efi_search_protocol(
> > + bm_handle, &efi_guid_device_path, &handler);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + ret = efi_protocol_open(handler, (void **)&device_path,
> > + efi_root, NULL,
> > + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + file_path = expand_media_path(device_path);
> > + ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > + NULL, 0, handle));
> > + efi_free_pool(file_path);
> > + if (ret == EFI_SUCCESS)
> > + goto out;
> > + }
> > + }
> > +
> > +out:
> > + efi_free_pool(label);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * try_load_from_uri_path() - Handle the URI device path
> > + *
> > + * @uridp: uri device path
> > + * @lo_label label of load option
> > + * @handle: pointer to handle for newly installed image
> > + * Return: status code
> > + */
> > +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> > + u16 *lo_label,
> > + efi_handle_t *handle)
> > +{
> > + efi_status_t ret;
> > + int file_size, file_name_len;
> > + char *s, *host_name, *file_name, *str_copy;
> > +
>
> From here should be normal caode:
OK.
>
> > + /*
> > + * Download file using wget.
> > + *
> > + * URI device path content is like http://www.example.com/sample/test.iso.
> > + * U-Boot wget takes the target uri in this format.
> > + * "<http server ip>:<file path>" e.g.) 192.168.1.1:/sample/test.iso
> > + * Need to resolve the http server ip address before starting wget.
> > + */
> > +
> > + /* only support "http://" */
> > + if (strncmp(uridp->uri, "http://", 7)) {
> > + log_err("Error: uri must start with http://\n");
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + str_copy = strdup(uridp->uri);
> > + if (!str_copy)
> > + return EFI_OUT_OF_RESOURCES;
> > + s = str_copy + strlen("http://");
> > + host_name = strsep(&s, "/");
> > + if (!s) {
> > + log_err("Error: invalied uri, no file path\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + file_name = s;
> > + net_dns_resolve = host_name;
> > + net_dns_env_var = "httpserverip";
> > + if (net_loop(DNS) < 0) {
> > + log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + s = env_get("httpserverip");
> > + if (!s) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /*
> > + * WGET requires that "net_boot_file_name" and "image_load_addr" global
> > + * variables are properly set in advance.
> > + */
> > + strlcpy(net_boot_file_name, s, 1024);
> > + strlcat(net_boot_file_name, ":/", 1024); /* append '/' which is removed by strsep() */
> > + strlcat(net_boot_file_name, file_name, 1024);
> > + s = env_get("loadaddr");
> > + if (!s) {
> > + log_err("Error: loadaddr is not set\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + image_load_addr = hextoul(s, NULL);
> > +
> > + file_size = net_loop(WGET);
> > + if (file_size < 0) {
> > + log_err("Error: downloading file failed\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Identify file type by file extension.
> > + * If the file extension is ".iso" or ".img", mount it and boot with default file.
> > + * If the file is ".efi", load and start the downloaded file.
> > + */
> > + file_name_len = strlen(net_boot_file_name);
>
> to here.
>
> You could package that up into a function which returns the filename.
> Then the EFI code is this:
OK.
>
> > + if (!strncmp(&net_boot_file_name[file_name_len - 4], ".iso", 4) ||
> > + !strncmp(&net_boot_file_name[file_name_len - 4], ".img", 4)) {
> > + ret = mount_image(lo_label, file_size, handle);
> > + } else if (!strncmp(&net_boot_file_name[file_name_len - 4], ".efi", 4)) {
> > + ret = efi_run_image((void *)image_load_addr, file_size);
> > + } else {
> > + log_err("Error: file type is not supported\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + }
> > +
> > +out:
> > + free(str_copy);
> > +
> > + return ret;
> > +}
> > +#endif
> > +
> > /**
> > * try_load_entry() - try to load image for boot option
> > *
> > @@ -211,6 +418,12 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> > if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
> > /* file_path doesn't contain a device path */
> > ret = try_load_from_short_path(lo.file_path, handle);
> > +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> > + } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
> P> + ret = try_load_from_uri_path(
> > + (struct efi_device_path_uri *)lo.file_path,
> > + lo.label, handle);
> > +#endif
> > } else {
> > file_path = expand_media_path(lo.file_path);
> > ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > --
> > 2.34.1
> >
>
> Also please consider how to test this. With the changes I mention
> above, you can fairly easily test the partition stuff (we have several
> mmc images for testing).
Yes. I wonder how to test the dns/wget part.
Thanks,
Masahisa Kojima
>
> Regards,
> Simon
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] efi_loader: support boot from URI device path
2023-08-23 8:37 ` [PATCH 2/2] efi_loader: support boot from URI " Masahisa Kojima
2023-08-23 23:57 ` Simon Glass
@ 2023-08-24 2:24 ` AKASHI Takahiro
2023-08-24 2:32 ` AKASHI Takahiro
2023-08-25 6:40 ` Masahisa Kojima
2023-08-24 6:58 ` Heinrich Schuchardt
2 siblings, 2 replies; 16+ messages in thread
From: AKASHI Takahiro @ 2023-08-24 2:24 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas
Kojima-san,
On Wed, Aug 23, 2023 at 05:37:20PM +0900, Masahisa Kojima wrote:
> This supports to boot from the URI device path.
> When user selects the URI device path, bootmgr downloads
> the file using wget into the address specified by loadaddr
> env variable.
> If the file is .iso or .img file, mount the image with blkmap
> then try to boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> If the file is .efi file, load and start the downloaded file.
Is this behavior part of UEFI specification?
Even so, it would be better to describe it in uefi.rst (or else),
including URI usage.
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> lib/efi_loader/efi_bootmgr.c | 213 +++++++++++++++++++++++++++++++++++
> 1 file changed, 213 insertions(+)
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a40762c74c..8b20f486f2 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -7,10 +7,14 @@
>
> #define LOG_CATEGORY LOGC_EFI
>
> +#include <blk.h>
> +#include <blkmap.h>
> #include <common.h>
> #include <charset.h>
> +#include <dm.h>
> #include <log.h>
> #include <malloc.h>
> +#include <net.h>
> #include <efi_default_filename.h>
> #include <efi_loader.h>
> #include <efi_variable.h>
> @@ -168,6 +172,209 @@ out:
> return ret;
> }
>
> +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> +/**
> + * mount_image() - mount the image
> + *
> + * @lo_label label of load option
> + * @file_size file size
> + * @handle: pointer to handle for newly installed image
> + * Return: status code
> + */
> +static efi_status_t mount_image(u16 *lo_label, int file_size,
> + efi_handle_t *handle)
I wonder why not adding "address" parameter to make this function
more generic as Simon suggested.
> +{
> + int err;
> + efi_status_t ret;
> + char *label = NULL, *p;
> + lbaint_t blknum;
> + struct udevice *bm_dev;
> + efi_handle_t bm_handle;
> + struct udevice *blk, *partition;
> + struct efi_handler *handler;
> + struct efi_device_path *file_path;
> + struct efi_device_path *device_path;
> +
> + label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> + if (!label)
> + return EFI_OUT_OF_RESOURCES;
> +
> + p = label;
> + utf16_utf8_strcpy(&p, lo_label);
> + err = blkmap_create(label, NULL);
> + if (err) {
> + log_err("failed to create blkmap\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + bm_dev = blkmap_from_label(label);
> + if (!bm_dev) {
> + log_err("\"%s\" is not the name of any known blkmap\n", label);
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + blknum = file_size / 512; /* TODO: don't use literal value. */
> + err = blkmap_map_pmem(bm_dev, 0, blknum, image_load_addr);
> + if (err) {
> + log_err("Unable to map %#llx at block %d : %d\n",
> + (unsigned long long)image_load_addr, 0, err);
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + log_info("Block %d+0x" LBAF " mapped to %#llx\n", 0, blknum,
> + (unsigned long long)image_load_addr);
> +
> + /* TODO: without calling this, partition devices are not binded. */
> + blk_list_part(UCLASS_BLKMAP);
I think that blkmap should provide a way to *probe* its child block
device. In fact,
struct blkmap *bm = dev_get_plat(bm_dev);
device_probe(bm->blk);
should work. (Not tested though)
> +
> + /*
> + * Search the partition having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> + */
> + device_foreach_child(blk, bm_dev)
Does bm_dev have more than one block devices?
> + {
> + device_foreach_child(partition, blk)
> + {
> + if (dev_tag_get_ptr(partition, DM_TAG_EFI,
> + (void **)&bm_handle)) {
> + log_warning("DM_TAG_EFI not found\n");
> + continue;
> + }
> +
> + ret = efi_search_protocol(
> + bm_handle,
> + &efi_simple_file_system_protocol_guid,
> + &handler);
> + if (ret != EFI_SUCCESS)
> + continue;
> +
> + ret = efi_search_protocol(
> + bm_handle, &efi_guid_device_path, &handler);
> + if (ret != EFI_SUCCESS)
> + continue;
> +
> + ret = efi_protocol_open(handler, (void **)&device_path,
> + efi_root, NULL,
> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> + if (ret != EFI_SUCCESS)
> + continue;
> +
> + file_path = expand_media_path(device_path);
> + ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> + NULL, 0, handle));
> + efi_free_pool(file_path);
> + if (ret == EFI_SUCCESS)
> + goto out;
> + }
> + }
Another idea would be to create a new boot option (as a removable
media) and to invoke it from bootmgr.
> +
> +out:
> + efi_free_pool(label);
> +
> + return ret;
> +}
> +
> +/**
> + * try_load_from_uri_path() - Handle the URI device path
> + *
> + * @uridp: uri device path
> + * @lo_label label of load option
> + * @handle: pointer to handle for newly installed image
> + * Return: status code
> + */
> +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> + u16 *lo_label,
> + efi_handle_t *handle)
> +{
> + efi_status_t ret;
> + int file_size, file_name_len;
> + char *s, *host_name, *file_name, *str_copy;
> +
> + /*
> + * Download file using wget.
> + *
> + * URI device path content is like http://www.example.com/sample/test.iso.
> + * U-Boot wget takes the target uri in this format.
> + * "<http server ip>:<file path>" e.g.) 192.168.1.1:/sample/test.iso
> + * Need to resolve the http server ip address before starting wget.
> + */
> +
> + /* only support "http://" */
> + if (strncmp(uridp->uri, "http://", 7)) {
> + log_err("Error: uri must start with http://\n");
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + str_copy = strdup(uridp->uri);
> + if (!str_copy)
> + return EFI_OUT_OF_RESOURCES;
> + s = str_copy + strlen("http://");
> + host_name = strsep(&s, "/");
> + if (!s) {
> + log_err("Error: invalied uri, no file path\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + file_name = s;
> + net_dns_resolve = host_name;
> + net_dns_env_var = "httpserverip";
> + if (net_loop(DNS) < 0) {
> + log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + s = env_get("httpserverip");
> + if (!s) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + /*
> + * WGET requires that "net_boot_file_name" and "image_load_addr" global
> + * variables are properly set in advance.
> + */
> + strlcpy(net_boot_file_name, s, 1024);
> + strlcat(net_boot_file_name, ":/", 1024); /* append '/' which is removed by strsep() */
> + strlcat(net_boot_file_name, file_name, 1024);
> + s = env_get("loadaddr");
> + if (!s) {
> + log_err("Error: loadaddr is not set\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + image_load_addr = hextoul(s, NULL);
> +
> + file_size = net_loop(WGET);
> + if (file_size < 0) {
> + log_err("Error: downloading file failed\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + /*
> + * Identify file type by file extension.
> + * If the file extension is ".iso" or ".img", mount it and boot with default file.
> + * If the file is ".efi", load and start the downloaded file.
> + */
> + file_name_len = strlen(net_boot_file_name);
> + if (!strncmp(&net_boot_file_name[file_name_len - 4], ".iso", 4) ||
> + !strncmp(&net_boot_file_name[file_name_len - 4], ".img", 4)) {
> + ret = mount_image(lo_label, file_size, handle);
> + } else if (!strncmp(&net_boot_file_name[file_name_len - 4], ".efi", 4)) {
> + ret = efi_run_image((void *)image_load_addr, file_size);
Nak.
try_load_from_uri_path() is called from try_load_entry().
This function's purpose is solely to load an image.
The execution should be triggered later.
-Takahiro Akashi
> + } else {
> + log_err("Error: file type is not supported\n");
> + ret = EFI_INVALID_PARAMETER;
> + }
> +
> +out:
> + free(str_copy);
> +
> + return ret;
> +}
> +#endif
> +
> /**
> * try_load_entry() - try to load image for boot option
> *
> @@ -211,6 +418,12 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
> /* file_path doesn't contain a device path */
> ret = try_load_from_short_path(lo.file_path, handle);
> +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> + } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
> + ret = try_load_from_uri_path(
> + (struct efi_device_path_uri *)lo.file_path,
> + lo.label, handle);
> +#endif
> } else {
> file_path = expand_media_path(lo.file_path);
> ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] efi_loader: support boot from URI device path
2023-08-24 2:24 ` AKASHI Takahiro
@ 2023-08-24 2:32 ` AKASHI Takahiro
2023-08-25 6:40 ` Masahisa Kojima
1 sibling, 0 replies; 16+ messages in thread
From: AKASHI Takahiro @ 2023-08-24 2:32 UTC (permalink / raw)
To: Masahisa Kojima, u-boot, Heinrich Schuchardt, Ilias Apalodimas
On Thu, Aug 24, 2023 at 11:24:31AM +0900, AKASHI Takahiro wrote:
> Kojima-san,
>
> On Wed, Aug 23, 2023 at 05:37:20PM +0900, Masahisa Kojima wrote:
> > This supports to boot from the URI device path.
> > When user selects the URI device path, bootmgr downloads
> > the file using wget into the address specified by loadaddr
> > env variable.
> > If the file is .iso or .img file, mount the image with blkmap
> > then try to boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > If the file is .efi file, load and start the downloaded file.
>
> Is this behavior part of UEFI specification?
> Even so, it would be better to describe it in uefi.rst (or else),
> including URI usage.
>
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > lib/efi_loader/efi_bootmgr.c | 213 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 213 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a40762c74c..8b20f486f2 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -7,10 +7,14 @@
> >
> > #define LOG_CATEGORY LOGC_EFI
> >
> > +#include <blk.h>
> > +#include <blkmap.h>
> > #include <common.h>
> > #include <charset.h>
> > +#include <dm.h>
> > #include <log.h>
> > #include <malloc.h>
> > +#include <net.h>
> > #include <efi_default_filename.h>
> > #include <efi_loader.h>
> > #include <efi_variable.h>
> > @@ -168,6 +172,209 @@ out:
> > return ret;
> > }
> >
> > +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> > +/**
> > + * mount_image() - mount the image
> > + *
> > + * @lo_label label of load option
> > + * @file_size file size
> > + * @handle: pointer to handle for newly installed image
> > + * Return: status code
> > + */
> > +static efi_status_t mount_image(u16 *lo_label, int file_size,
> > + efi_handle_t *handle)
>
> I wonder why not adding "address" parameter to make this function
> more generic as Simon suggested.
>
> > +{
> > + int err;
> > + efi_status_t ret;
> > + char *label = NULL, *p;
> > + lbaint_t blknum;
> > + struct udevice *bm_dev;
> > + efi_handle_t bm_handle;
> > + struct udevice *blk, *partition;
> > + struct efi_handler *handler;
> > + struct efi_device_path *file_path;
> > + struct efi_device_path *device_path;
> > +
> > + label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> > + if (!label)
> > + return EFI_OUT_OF_RESOURCES;
> > +
> > + p = label;
> > + utf16_utf8_strcpy(&p, lo_label);
> > + err = blkmap_create(label, NULL);
> > + if (err) {
> > + log_err("failed to create blkmap\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + bm_dev = blkmap_from_label(label);
> > + if (!bm_dev) {
> > + log_err("\"%s\" is not the name of any known blkmap\n", label);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + blknum = file_size / 512; /* TODO: don't use literal value. */
> > + err = blkmap_map_pmem(bm_dev, 0, blknum, image_load_addr);
> > + if (err) {
> > + log_err("Unable to map %#llx at block %d : %d\n",
> > + (unsigned long long)image_load_addr, 0, err);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + log_info("Block %d+0x" LBAF " mapped to %#llx\n", 0, blknum,
> > + (unsigned long long)image_load_addr);
> > +
> > + /* TODO: without calling this, partition devices are not binded. */
> > + blk_list_part(UCLASS_BLKMAP);
>
> I think that blkmap should provide a way to *probe* its child block
> device. In fact,
> struct blkmap *bm = dev_get_plat(bm_dev);
> device_probe(bm->blk);
> should work. (Not tested though)
>
> > +
> > + /*
> > + * Search the partition having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> > + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > + */
> > + device_foreach_child(blk, bm_dev)
>
> Does bm_dev have more than one block devices?
>
> > + {
> > + device_foreach_child(partition, blk)
> > + {
> > + if (dev_tag_get_ptr(partition, DM_TAG_EFI,
> > + (void **)&bm_handle)) {
> > + log_warning("DM_TAG_EFI not found\n");
> > + continue;
> > + }
> > +
> > + ret = efi_search_protocol(
> > + bm_handle,
> > + &efi_simple_file_system_protocol_guid,
> > + &handler);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + ret = efi_search_protocol(
> > + bm_handle, &efi_guid_device_path, &handler);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + ret = efi_protocol_open(handler, (void **)&device_path,
> > + efi_root, NULL,
> > + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + file_path = expand_media_path(device_path);
> > + ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > + NULL, 0, handle));
> > + efi_free_pool(file_path);
> > + if (ret == EFI_SUCCESS)
> > + goto out;
> > + }
> > + }
>
> Another idea would be to create a new boot option (as a removable
> media) and to invoke it from bootmgr.
Never mind this comment.
-Takahiro Akashi
>
> > +
> > +out:
> > + efi_free_pool(label);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * try_load_from_uri_path() - Handle the URI device path
> > + *
> > + * @uridp: uri device path
> > + * @lo_label label of load option
> > + * @handle: pointer to handle for newly installed image
> > + * Return: status code
> > + */
> > +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> > + u16 *lo_label,
> > + efi_handle_t *handle)
> > +{
> > + efi_status_t ret;
> > + int file_size, file_name_len;
> > + char *s, *host_name, *file_name, *str_copy;
> > +
> > + /*
> > + * Download file using wget.
> > + *
> > + * URI device path content is like http://www.example.com/sample/test.iso.
> > + * U-Boot wget takes the target uri in this format.
> > + * "<http server ip>:<file path>" e.g.) 192.168.1.1:/sample/test.iso
> > + * Need to resolve the http server ip address before starting wget.
> > + */
> > +
> > + /* only support "http://" */
> > + if (strncmp(uridp->uri, "http://", 7)) {
> > + log_err("Error: uri must start with http://\n");
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + str_copy = strdup(uridp->uri);
> > + if (!str_copy)
> > + return EFI_OUT_OF_RESOURCES;
> > + s = str_copy + strlen("http://");
> > + host_name = strsep(&s, "/");
> > + if (!s) {
> > + log_err("Error: invalied uri, no file path\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + file_name = s;
> > + net_dns_resolve = host_name;
> > + net_dns_env_var = "httpserverip";
> > + if (net_loop(DNS) < 0) {
> > + log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + s = env_get("httpserverip");
> > + if (!s) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /*
> > + * WGET requires that "net_boot_file_name" and "image_load_addr" global
> > + * variables are properly set in advance.
> > + */
> > + strlcpy(net_boot_file_name, s, 1024);
> > + strlcat(net_boot_file_name, ":/", 1024); /* append '/' which is removed by strsep() */
> > + strlcat(net_boot_file_name, file_name, 1024);
> > + s = env_get("loadaddr");
> > + if (!s) {
> > + log_err("Error: loadaddr is not set\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + image_load_addr = hextoul(s, NULL);
> > +
> > + file_size = net_loop(WGET);
> > + if (file_size < 0) {
> > + log_err("Error: downloading file failed\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Identify file type by file extension.
> > + * If the file extension is ".iso" or ".img", mount it and boot with default file.
> > + * If the file is ".efi", load and start the downloaded file.
> > + */
> > + file_name_len = strlen(net_boot_file_name);
> > + if (!strncmp(&net_boot_file_name[file_name_len - 4], ".iso", 4) ||
> > + !strncmp(&net_boot_file_name[file_name_len - 4], ".img", 4)) {
> > + ret = mount_image(lo_label, file_size, handle);
> > + } else if (!strncmp(&net_boot_file_name[file_name_len - 4], ".efi", 4)) {
> > + ret = efi_run_image((void *)image_load_addr, file_size);
>
> Nak.
> try_load_from_uri_path() is called from try_load_entry().
> This function's purpose is solely to load an image.
> The execution should be triggered later.
>
> -Takahiro Akashi
>
>
> > + } else {
> > + log_err("Error: file type is not supported\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + }
> > +
> > +out:
> > + free(str_copy);
> > +
> > + return ret;
> > +}
> > +#endif
> > +
> > /**
> > * try_load_entry() - try to load image for boot option
> > *
> > @@ -211,6 +418,12 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> > if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
> > /* file_path doesn't contain a device path */
> > ret = try_load_from_short_path(lo.file_path, handle);
> > +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> > + } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
> > + ret = try_load_from_uri_path(
> > + (struct efi_device_path_uri *)lo.file_path,
> > + lo.label, handle);
> > +#endif
> > } else {
> > file_path = expand_media_path(lo.file_path);
> > ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] efi_loader: support boot from URI device path
2023-08-24 2:24 ` AKASHI Takahiro
2023-08-24 2:32 ` AKASHI Takahiro
@ 2023-08-25 6:40 ` Masahisa Kojima
1 sibling, 0 replies; 16+ messages in thread
From: Masahisa Kojima @ 2023-08-25 6:40 UTC (permalink / raw)
To: AKASHI Takahiro, Masahisa Kojima, u-boot, Heinrich Schuchardt,
Ilias Apalodimas
On Thu, 24 Aug 2023 at 11:24, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Kojima-san,
>
> On Wed, Aug 23, 2023 at 05:37:20PM +0900, Masahisa Kojima wrote:
> > This supports to boot from the URI device path.
> > When user selects the URI device path, bootmgr downloads
> > the file using wget into the address specified by loadaddr
> > env variable.
> > If the file is .iso or .img file, mount the image with blkmap
> > then try to boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > If the file is .efi file, load and start the downloaded file.
>
> Is this behavior part of UEFI specification?
> Even so, it would be better to describe it in uefi.rst (or else),
> including URI usage.
Yes, I will update the documentation.
>
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > lib/efi_loader/efi_bootmgr.c | 213 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 213 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a40762c74c..8b20f486f2 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -7,10 +7,14 @@
> >
> > #define LOG_CATEGORY LOGC_EFI
> >
> > +#include <blk.h>
> > +#include <blkmap.h>
> > #include <common.h>
> > #include <charset.h>
> > +#include <dm.h>
> > #include <log.h>
> > #include <malloc.h>
> > +#include <net.h>
> > #include <efi_default_filename.h>
> > #include <efi_loader.h>
> > #include <efi_variable.h>
> > @@ -168,6 +172,209 @@ out:
> > return ret;
> > }
> >
> > +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> > +/**
> > + * mount_image() - mount the image
> > + *
> > + * @lo_label label of load option
> > + * @file_size file size
> > + * @handle: pointer to handle for newly installed image
> > + * Return: status code
> > + */
> > +static efi_status_t mount_image(u16 *lo_label, int file_size,
> > + efi_handle_t *handle)
>
> I wonder why not adding "address" parameter to make this function
> more generic as Simon suggested.
wget and other network commands take the load address with the
image_load_address
global variable. Anyway, adding address as parameter is better.
>
> > +{
> > + int err;
> > + efi_status_t ret;
> > + char *label = NULL, *p;
> > + lbaint_t blknum;
> > + struct udevice *bm_dev;
> > + efi_handle_t bm_handle;
> > + struct udevice *blk, *partition;
> > + struct efi_handler *handler;
> > + struct efi_device_path *file_path;
> > + struct efi_device_path *device_path;
> > +
> > + label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> > + if (!label)
> > + return EFI_OUT_OF_RESOURCES;
> > +
> > + p = label;
> > + utf16_utf8_strcpy(&p, lo_label);
> > + err = blkmap_create(label, NULL);
> > + if (err) {
> > + log_err("failed to create blkmap\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + bm_dev = blkmap_from_label(label);
> > + if (!bm_dev) {
> > + log_err("\"%s\" is not the name of any known blkmap\n", label);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + blknum = file_size / 512; /* TODO: don't use literal value. */
> > + err = blkmap_map_pmem(bm_dev, 0, blknum, image_load_addr);
> > + if (err) {
> > + log_err("Unable to map %#llx at block %d : %d\n",
> > + (unsigned long long)image_load_addr, 0, err);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + log_info("Block %d+0x" LBAF " mapped to %#llx\n", 0, blknum,
> > + (unsigned long long)image_load_addr);
> > +
> > + /* TODO: without calling this, partition devices are not binded. */
> > + blk_list_part(UCLASS_BLKMAP);
>
> I think that blkmap should provide a way to *probe* its child block
> device. In fact,
> struct blkmap *bm = dev_get_plat(bm_dev);
> device_probe(bm->blk);
> should work. (Not tested though)
Thank you. I implemented this initially but did not work as expected.
I will check again.
>
> > +
> > + /*
> > + * Search the partition having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> > + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > + */
> > + device_foreach_child(blk, bm_dev)
>
> Does bm_dev have more than one block devices?
No, I can remove foreach_child.
>
> > + {
> > + device_foreach_child(partition, blk)
> > + {
> > + if (dev_tag_get_ptr(partition, DM_TAG_EFI,
> > + (void **)&bm_handle)) {
> > + log_warning("DM_TAG_EFI not found\n");
> > + continue;
> > + }
> > +
> > + ret = efi_search_protocol(
> > + bm_handle,
> > + &efi_simple_file_system_protocol_guid,
> > + &handler);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + ret = efi_search_protocol(
> > + bm_handle, &efi_guid_device_path, &handler);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + ret = efi_protocol_open(handler, (void **)&device_path,
> > + efi_root, NULL,
> > + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + file_path = expand_media_path(device_path);
> > + ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > + NULL, 0, handle));
> > + efi_free_pool(file_path);
> > + if (ret == EFI_SUCCESS)
> > + goto out;
> > + }
> > + }
>
> Another idea would be to create a new boot option (as a removable
> media) and to invoke it from bootmgr.
>
> > +
> > +out:
> > + efi_free_pool(label);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * try_load_from_uri_path() - Handle the URI device path
> > + *
> > + * @uridp: uri device path
> > + * @lo_label label of load option
> > + * @handle: pointer to handle for newly installed image
> > + * Return: status code
> > + */
> > +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> > + u16 *lo_label,
> > + efi_handle_t *handle)
> > +{
> > + efi_status_t ret;
> > + int file_size, file_name_len;
> > + char *s, *host_name, *file_name, *str_copy;
> > +
> > + /*
> > + * Download file using wget.
> > + *
> > + * URI device path content is like http://www.example.com/sample/test.iso.
> > + * U-Boot wget takes the target uri in this format.
> > + * "<http server ip>:<file path>" e.g.) 192.168.1.1:/sample/test.iso
> > + * Need to resolve the http server ip address before starting wget.
> > + */
> > +
> > + /* only support "http://" */
> > + if (strncmp(uridp->uri, "http://", 7)) {
> > + log_err("Error: uri must start with http://\n");
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + str_copy = strdup(uridp->uri);
> > + if (!str_copy)
> > + return EFI_OUT_OF_RESOURCES;
> > + s = str_copy + strlen("http://");
> > + host_name = strsep(&s, "/");
> > + if (!s) {
> > + log_err("Error: invalied uri, no file path\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + file_name = s;
> > + net_dns_resolve = host_name;
> > + net_dns_env_var = "httpserverip";
> > + if (net_loop(DNS) < 0) {
> > + log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + s = env_get("httpserverip");
> > + if (!s) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /*
> > + * WGET requires that "net_boot_file_name" and "image_load_addr" global
> > + * variables are properly set in advance.
> > + */
> > + strlcpy(net_boot_file_name, s, 1024);
> > + strlcat(net_boot_file_name, ":/", 1024); /* append '/' which is removed by strsep() */
> > + strlcat(net_boot_file_name, file_name, 1024);
> > + s = env_get("loadaddr");
> > + if (!s) {
> > + log_err("Error: loadaddr is not set\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + image_load_addr = hextoul(s, NULL);
> > +
> > + file_size = net_loop(WGET);
> > + if (file_size < 0) {
> > + log_err("Error: downloading file failed\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Identify file type by file extension.
> > + * If the file extension is ".iso" or ".img", mount it and boot with default file.
> > + * If the file is ".efi", load and start the downloaded file.
> > + */
> > + file_name_len = strlen(net_boot_file_name);
> > + if (!strncmp(&net_boot_file_name[file_name_len - 4], ".iso", 4) ||
> > + !strncmp(&net_boot_file_name[file_name_len - 4], ".img", 4)) {
> > + ret = mount_image(lo_label, file_size, handle);
> > + } else if (!strncmp(&net_boot_file_name[file_name_len - 4], ".efi", 4)) {
> > + ret = efi_run_image((void *)image_load_addr, file_size);
>
> Nak.
> try_load_from_uri_path() is called from try_load_entry().
> This function's purpose is solely to load an image.
> The execution should be triggered later.
Yes, you are correct.
I will fix this.
Thanks,
Masahisa Kojima
>
> -Takahiro Akashi
>
>
> > + } else {
> > + log_err("Error: file type is not supported\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + }
> > +
> > +out:
> > + free(str_copy);
> > +
> > + return ret;
> > +}
> > +#endif
> > +
> > /**
> > * try_load_entry() - try to load image for boot option
> > *
> > @@ -211,6 +418,12 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> > if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
> > /* file_path doesn't contain a device path */
> > ret = try_load_from_short_path(lo.file_path, handle);
> > +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> > + } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
> > + ret = try_load_from_uri_path(
> > + (struct efi_device_path_uri *)lo.file_path,
> > + lo.label, handle);
> > +#endif
> > } else {
> > file_path = expand_media_path(lo.file_path);
> > ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > --
> > 2.34.1
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] efi_loader: support boot from URI device path
2023-08-23 8:37 ` [PATCH 2/2] efi_loader: support boot from URI " Masahisa Kojima
2023-08-23 23:57 ` Simon Glass
2023-08-24 2:24 ` AKASHI Takahiro
@ 2023-08-24 6:58 ` Heinrich Schuchardt
2023-08-25 7:56 ` Masahisa Kojima
2 siblings, 1 reply; 16+ messages in thread
From: Heinrich Schuchardt @ 2023-08-24 6:58 UTC (permalink / raw)
To: Masahisa Kojima; +Cc: Ilias Apalodimas, u-boot
On 8/23/23 10:37, Masahisa Kojima wrote:
> This supports to boot from the URI device path.
> When user selects the URI device path, bootmgr downloads
> the file using wget into the address specified by loadaddr
> env variable.
> If the file is .iso or .img file, mount the image with blkmap
> then try to boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> If the file is .efi file, load and start the downloaded file.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> lib/efi_loader/efi_bootmgr.c | 213 +++++++++++++++++++++++++++++++++++
> 1 file changed, 213 insertions(+)
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a40762c74c..8b20f486f2 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -7,10 +7,14 @@
>
> #define LOG_CATEGORY LOGC_EFI
>
> +#include <blk.h>
> +#include <blkmap.h>
> #include <common.h>
> #include <charset.h>
> +#include <dm.h>
> #include <log.h>
> #include <malloc.h>
> +#include <net.h>
> #include <efi_default_filename.h>
> #include <efi_loader.h>
> #include <efi_variable.h>
> @@ -168,6 +172,209 @@ out:
> return ret;
> }
>
> +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> +/**
> + * mount_image() - mount the image
> + *
> + * @lo_label label of load option
> + * @file_size file size
> + * @handle: pointer to handle for newly installed image
> + * Return: status code
> + */
> +static efi_status_t mount_image(u16 *lo_label, int file_size,
> + efi_handle_t *handle)
> +{
> + int err;
> + efi_status_t ret;
> + char *label = NULL, *p;
> + lbaint_t blknum;
> + struct udevice *bm_dev;
> + efi_handle_t bm_handle;
> + struct udevice *blk, *partition;
> + struct efi_handler *handler;
> + struct efi_device_path *file_path;
> + struct efi_device_path *device_path;
> +
> + label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> + if (!label)
> + return EFI_OUT_OF_RESOURCES;
> +
> + p = label;
> + utf16_utf8_strcpy(&p, lo_label);
> + err = blkmap_create(label, NULL);
> + if (err) {
> + log_err("failed to create blkmap\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + bm_dev = blkmap_from_label(label);
> + if (!bm_dev) {
> + log_err("\"%s\" is not the name of any known blkmap\n", label);
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + blknum = file_size / 512; /* TODO: don't use literal value. */
Can't you retrieve the block size from the udevice?
> + err = blkmap_map_pmem(bm_dev, 0, blknum, image_load_addr);
> + if (err) {
> + log_err("Unable to map %#llx at block %d : %d\n",
> + (unsigned long long)image_load_addr, 0, err);
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + log_info("Block %d+0x" LBAF " mapped to %#llx\n", 0, blknum,
> + (unsigned long long)image_load_addr);
> +
> + /* TODO: without calling this, partition devices are not binded. */
%s/binded/bound/
> + blk_list_part(UCLASS_BLKMAP);
Why would you want to display all BLKMAP devices?
Please, avoid unnecessary output.
> +
> + /*
> + * Search the partition having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> + */
> + device_foreach_child(blk, bm_dev)
> + {
You need to check that blk is of type UCLASS_PARTITION.
What about images that have no partition table but only a file system?
> + device_foreach_child(partition, blk)
> + {
> + if (dev_tag_get_ptr(partition, DM_TAG_EFI,
> + (void **)&bm_handle)) {
> + log_warning("DM_TAG_EFI not found\n");
> + continue;
> + }
> +
> + ret = efi_search_protocol(
> + bm_handle,
> + &efi_simple_file_system_protocol_guid,
> + &handler);
> + if (ret != EFI_SUCCESS)
> + continue;
> +
> + ret = efi_search_protocol(
> + bm_handle, &efi_guid_device_path, &handler);
> + if (ret != EFI_SUCCESS)
> + continue;
> +
> + ret = efi_protocol_open(handler, (void **)&device_path,
> + efi_root, NULL,
> + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> + if (ret != EFI_SUCCESS)
> + continue;
Do you expect multiple ESPs? Why not return the error here?
> +
> + file_path = expand_media_path(device_path);
> + ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> + NULL, 0, handle));
> + efi_free_pool(file_path);
> + if (ret == EFI_SUCCESS)
> + goto out;
ditto
> + }
> + }
> +
ret may not even be initialized at this point!
I would expect EFI_NOT_FOUND to be returned if there is no ESP.
> +out:
> + efi_free_pool(label);
> +
> + return ret;
> +}
> +
> +/**
> + * try_load_from_uri_path() - Handle the URI device path
> + *
> + * @uridp: uri device path
> + * @lo_label label of load option
> + * @handle: pointer to handle for newly installed image
> + * Return: status code
> + */
> +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> + u16 *lo_label,
> + efi_handle_t *handle)
> +{
> + efi_status_t ret;
> + int file_size, file_name_len;
> + char *s, *host_name, *file_name, *str_copy;
> +
> + /*
> + * Download file using wget.
> + *
> + * URI device path content is like http://www.example.com/sample/test.iso.
> + * U-Boot wget takes the target uri in this format.
> + * "<http server ip>:<file path>" e.g.) 192.168.1.1:/sample/test.iso
> + * Need to resolve the http server ip address before starting wget.
> + */
> +
> + /* only support "http://" */
> + if (strncmp(uridp->uri, "http://", 7)) {
> + log_err("Error: uri must start with http://\n");
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + str_copy = strdup(uridp->uri);
> + if (!str_copy)
> + return EFI_OUT_OF_RESOURCES;
> + s = str_copy + strlen("http://");
> + host_name = strsep(&s, "/");
This could be "user:password@example.com".
> + if (!s) {
> + log_err("Error: invalied uri, no file path\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + file_name = s;
> + net_dns_resolve = host_name;
> + net_dns_env_var = "httpserverip";
> + if (net_loop(DNS) < 0) {
Why call net_loop(DNS) for an IP address like
[2a00:1450:4001:812::200e] or 142.250.185.206?
> + log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
This logic seems not to be EFI related. There should be a network
library function that takes a URL and returns a filled buffer.
> + s = env_get("httpserverip");
Why should this variable be used if host_name is "142.250.185.206"?
If the host name has no DNS entry and is not a valid IP address we
should error out here.
> + if (!s) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + /*
> + * WGET requires that "net_boot_file_name" and "image_load_addr" global
> + * variables are properly set in advance.
> + */
> + strlcpy(net_boot_file_name, s, 1024);
> + strlcat(net_boot_file_name, ":/", 1024); /* append '/' which is removed by strsep() */
On a single IP address you may find multiple servers. Even if there is
only one it may not provide the resource if you don't supply the host name.
It would be preferable to adjust wget to comply to RFC 7320 ("Hypertext
Transfer Protocol (HTTP/1.1): Message Syntax and Routing") and provide a
HOST: header.
> + strlcat(net_boot_file_name, file_name, 1024);
> + s = env_get("loadaddr");
> + if (!s) {
> + log_err("Error: loadaddr is not set\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + image_load_addr = hextoul(s, NULL);
> +
> + file_size = net_loop(WGET);
This looks insecure.
You must define a maximum file size before trying to download and use
lmb_init_and_reserve() to check that the buffer is available. Otherwise
you might download a large file that overwrites the stack or U-Boot's code.
net_loop() must check that the reserved memory size is not exceeded.
> + if (file_size < 0) {
> + log_err("Error: downloading file failed\n");
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + /*
> + * Identify file type by file extension.
> + * If the file extension is ".iso" or ".img", mount it and boot with default file.
> + * If the file is ".efi", load and start the downloaded file.
Please, don't rely on file extensions.
Inspect the buffer using function efi_check_pe() to discover if it is an
EFI binary.
mount_image() should return an error code if the buffer does not contain
a partition table or a file system.
Best regards
Heinrich
> + */
> + file_name_len = strlen(net_boot_file_name);
> + if (!strncmp(&net_boot_file_name[file_name_len - 4], ".iso", 4) ||
> + !strncmp(&net_boot_file_name[file_name_len - 4], ".img", 4)) {
> + ret = mount_image(lo_label, file_size, handle);
> + } else if (!strncmp(&net_boot_file_name[file_name_len - 4], ".efi", 4)) {
> + ret = efi_run_image((void *)image_load_addr, file_size);
> + } else {
> + log_err("Error: file type is not supported\n");
> + ret = EFI_INVALID_PARAMETER;
> + }
> +
> +out:
> + free(str_copy);
> +
> + return ret;
> +}
> +#endif
> +
> /**
> * try_load_entry() - try to load image for boot option
> *
> @@ -211,6 +418,12 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
> /* file_path doesn't contain a device path */
> ret = try_load_from_short_path(lo.file_path, handle);
> +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> + } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
> + ret = try_load_from_uri_path(
> + (struct efi_device_path_uri *)lo.file_path,
> + lo.label, handle);
> +#endif
> } else {
> file_path = expand_media_path(lo.file_path);
> ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 2/2] efi_loader: support boot from URI device path
2023-08-24 6:58 ` Heinrich Schuchardt
@ 2023-08-25 7:56 ` Masahisa Kojima
0 siblings, 0 replies; 16+ messages in thread
From: Masahisa Kojima @ 2023-08-25 7:56 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Ilias Apalodimas, u-boot
On Thu, 24 Aug 2023 at 15:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 8/23/23 10:37, Masahisa Kojima wrote:
> > This supports to boot from the URI device path.
> > When user selects the URI device path, bootmgr downloads
> > the file using wget into the address specified by loadaddr
> > env variable.
> > If the file is .iso or .img file, mount the image with blkmap
> > then try to boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > If the file is .efi file, load and start the downloaded file.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > lib/efi_loader/efi_bootmgr.c | 213 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 213 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a40762c74c..8b20f486f2 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -7,10 +7,14 @@
> >
> > #define LOG_CATEGORY LOGC_EFI
> >
> > +#include <blk.h>
> > +#include <blkmap.h>
> > #include <common.h>
> > #include <charset.h>
> > +#include <dm.h>
> > #include <log.h>
> > #include <malloc.h>
> > +#include <net.h>
> > #include <efi_default_filename.h>
> > #include <efi_loader.h>
> > #include <efi_variable.h>
> > @@ -168,6 +172,209 @@ out:
> > return ret;
> > }
> >
> > +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> > +/**
> > + * mount_image() - mount the image
> > + *
> > + * @lo_label label of load option
> > + * @file_size file size
> > + * @handle: pointer to handle for newly installed image
> > + * Return: status code
> > + */
> > +static efi_status_t mount_image(u16 *lo_label, int file_size,
> > + efi_handle_t *handle)
> > +{
> > + int err;
> > + efi_status_t ret;
> > + char *label = NULL, *p;
> > + lbaint_t blknum;
> > + struct udevice *bm_dev;
> > + efi_handle_t bm_handle;
> > + struct udevice *blk, *partition;
> > + struct efi_handler *handler;
> > + struct efi_device_path *file_path;
> > + struct efi_device_path *device_path;
> > +
> > + label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> > + if (!label)
> > + return EFI_OUT_OF_RESOURCES;
> > +
> > + p = label;
> > + utf16_utf8_strcpy(&p, lo_label);
> > + err = blkmap_create(label, NULL);
> > + if (err) {
> > + log_err("failed to create blkmap\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + bm_dev = blkmap_from_label(label);
> > + if (!bm_dev) {
> > + log_err("\"%s\" is not the name of any known blkmap\n", label);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + blknum = file_size / 512; /* TODO: don't use literal value. */
>
> Can't you retrieve the block size from the udevice?
I have tried but I can't get block size. Anyway I will check again.
>
> > + err = blkmap_map_pmem(bm_dev, 0, blknum, image_load_addr);
> > + if (err) {
> > + log_err("Unable to map %#llx at block %d : %d\n",
> > + (unsigned long long)image_load_addr, 0, err);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + log_info("Block %d+0x" LBAF " mapped to %#llx\n", 0, blknum,
> > + (unsigned long long)image_load_addr);
> > +
> > + /* TODO: without calling this, partition devices are not binded. */
>
> %s/binded/bound/
Thank you for correcting the typo.
>
> > + blk_list_part(UCLASS_BLKMAP);
>
> Why would you want to display all BLKMAP devices?
> Please, avoid unnecessary output.
I will try to probe all the partitions with device_probe() call.
>
> > +
> > + /*
> > + * Search the partition having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> > + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > + */
> > + device_foreach_child(blk, bm_dev)
> > + {
>
> You need to check that blk is of type UCLASS_PARTITION.
OK.
>
> What about images that have no partition table but only a file system?
I will check how to handle this case.
>
> > + device_foreach_child(partition, blk)
> > + {
> > + if (dev_tag_get_ptr(partition, DM_TAG_EFI,
> > + (void **)&bm_handle)) {
> > + log_warning("DM_TAG_EFI not found\n");
> > + continue;
> > + }
> > +
> > + ret = efi_search_protocol(
> > + bm_handle,
> > + &efi_simple_file_system_protocol_guid,
> > + &handler);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + ret = efi_search_protocol(
> > + bm_handle, &efi_guid_device_path, &handler);
> > + if (ret != EFI_SUCCESS)
> > + continue;
> > +
> > + ret = efi_protocol_open(handler, (void **)&device_path,
> > + efi_root, NULL,
> > + EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > + if (ret != EFI_SUCCESS)
> > + continue;
>
> Do you expect multiple ESPs? Why not return the error here?
According to the UEFI spec, the system can boot from the device
having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
This loop does not try to find the ESP, try to find the device having
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and check if there is a
default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
>
> > +
> > + file_path = expand_media_path(device_path);
> > + ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > + NULL, 0, handle));
> > + efi_free_pool(file_path);
> > + if (ret == EFI_SUCCESS)
> > + goto out;
>
> ditto
At here, the default boot file is loaded into the memory, we could
exit the loop and
start the image.
>
> > + }
> > + }
> > +
>
> ret may not even be initialized at this point!
Thank you, I will fix this, EFI_NOT_FOUND should be returned.
> I would expect EFI_NOT_FOUND to be returned if there is no ESP.
My intention here is that there is no bootable device contains default
boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
>
> > +out:
> > + efi_free_pool(label);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * try_load_from_uri_path() - Handle the URI device path
> > + *
> > + * @uridp: uri device path
> > + * @lo_label label of load option
> > + * @handle: pointer to handle for newly installed image
> > + * Return: status code
> > + */
> > +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> > + u16 *lo_label,
> > + efi_handle_t *handle)
> > +{
> > + efi_status_t ret;
> > + int file_size, file_name_len;
> > + char *s, *host_name, *file_name, *str_copy;
> > +
> > + /*
> > + * Download file using wget.
> > + *
> > + * URI device path content is like http://www.example.com/sample/test.iso.
> > + * U-Boot wget takes the target uri in this format.
> > + * "<http server ip>:<file path>" e.g.) 192.168.1.1:/sample/test.iso
> > + * Need to resolve the http server ip address before starting wget.
> > + */
> > +
> > + /* only support "http://" */
> > + if (strncmp(uridp->uri, "http://", 7)) {
> > + log_err("Error: uri must start with http://\n");
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + str_copy = strdup(uridp->uri);
> > + if (!str_copy)
> > + return EFI_OUT_OF_RESOURCES;
> > + s = str_copy + strlen("http://");
> > + host_name = strsep(&s, "/");
>
> This could be "user:password@example.com".
Yes, but current wget does not support this format.
Need to be checked when the user input the URI as you suggested.
>
> > + if (!s) {
> > + log_err("Error: invalied uri, no file path\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + file_name = s;
> > + net_dns_resolve = host_name;
> > + net_dns_env_var = "httpserverip";
> > + if (net_loop(DNS) < 0) {
>
> Why call net_loop(DNS) for an IP address like
> [2a00:1450:4001:812::200e] or 142.250.185.206?
Currently lwip migration is ongoing, and lwip wget correctly handles this.
I plan to rebase on top of the lwip port.
>
> > + log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
>
> This logic seems not to be EFI related. There should be a network
> library function that takes a URL and returns a filled buffer.
>
> > + s = env_get("httpserverip");
>
> Why should this variable be used if host_name is "142.250.185.206"?
OK.
>
> If the host name has no DNS entry and is not a valid IP address we
> should error out here.
OK.
>
> > + if (!s) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /*
> > + * WGET requires that "net_boot_file_name" and "image_load_addr" global
> > + * variables are properly set in advance.
> > + */
> > + strlcpy(net_boot_file_name, s, 1024);
> > + strlcat(net_boot_file_name, ":/", 1024); /* append '/' which is removed by strsep() */
>
> On a single IP address you may find multiple servers. Even if there is
> only one it may not provide the resource if you don't supply the host name.
>
> It would be preferable to adjust wget to comply to RFC 7320 ("Hypertext
> Transfer Protocol (HTTP/1.1): Message Syntax and Routing") and provide a
> HOST: header.
Current wget does not support HOST: header but lwip port does support.
So lwip migration will address this issue.
>
> > + strlcat(net_boot_file_name, file_name, 1024);
> > + s = env_get("loadaddr");
> > + if (!s) {
> > + log_err("Error: loadaddr is not set\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + image_load_addr = hextoul(s, NULL);
> > +
> > + file_size = net_loop(WGET);
>
> This looks insecure.
>
> You must define a maximum file size before trying to download and use
> lmb_init_and_reserve() to check that the buffer is available. Otherwise
> you might download a large file that overwrites the stack or U-Boot's code.
>
> net_loop() must check that the reserved memory size is not exceeded.
OK, I will add the maximum file size env variable, or try to utilize the
Content-Length response header.
>
> > + if (file_size < 0) {
> > + log_err("Error: downloading file failed\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Identify file type by file extension.
> > + * If the file extension is ".iso" or ".img", mount it and boot with default file.
> > + * If the file is ".efi", load and start the downloaded file.
>
> Please, don't rely on file extensions.
>
> Inspect the buffer using function efi_check_pe() to discover if it is an
> EFI binary.
OK.
>
> mount_image() should return an error code if the buffer does not contain
> a partition table or a file system.
OK.
Thanks,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> > + */
> > + file_name_len = strlen(net_boot_file_name);
> > + if (!strncmp(&net_boot_file_name[file_name_len - 4], ".iso", 4) ||
> > + !strncmp(&net_boot_file_name[file_name_len - 4], ".img", 4)) {
> > + ret = mount_image(lo_label, file_size, handle);
> > + } else if (!strncmp(&net_boot_file_name[file_name_len - 4], ".efi", 4)) {
> > + ret = efi_run_image((void *)image_load_addr, file_size);
> > + } else {
> > + log_err("Error: file type is not supported\n");
> > + ret = EFI_INVALID_PARAMETER;
> > + }
> > +
> > +out:
> > + free(str_copy);
> > +
> > + return ret;
> > +}
> > +#endif
> > +
> > /**
> > * try_load_entry() - try to load image for boot option
> > *
> > @@ -211,6 +418,12 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
> > if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
> > /* file_path doesn't contain a device path */
> > ret = try_load_from_short_path(lo.file_path, handle);
> > +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> > + } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
> > + ret = try_load_from_uri_path(
> > + (struct efi_device_path_uri *)lo.file_path,
> > + lo.label, handle);
> > +#endif
> > } else {
> > file_path = expand_media_path(lo.file_path);
> > ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
>
^ permalink raw reply [flat|nested] 16+ messages in thread