[PATCH 0/2] Add EFI HTTP boot support

[PATCH 0/2] Add EFI HTTP boot support

[Masahisa Kojima]

This series adds the EFI HTTP boot support.
User can add the URI device path with "efidebug boot add" command.
efibootmgr handles the URI device path, download the
specified file using wget, mount the downloaded image with
blkmap, then boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).

To enable EFI HTTP boot, we need to enable the following Kconfig options.
 CONFIG_CMD_DNS
 CONFIG_CMD_WGET
 CONFIG_BLKMAP

On the Socionext Developerbox, enter the following commands then
debian installer is downloaded into "loadaddr" and installer
automatically starts.
 => dhcp
 => setenv serverip 192.168.1.1
 => efidebug boot add -u 3 debian-netinst http://ftp.riken.jp/Linux/debian/debian-cd/12.1.0/arm64/iso-cd/debian-12.1.0-arm64-netinst.iso
 => efidebug boot order 3
 => bootefi bootmgr

Note that this debian installer can not proceed the installation
bacause RAM disk of installer image is not recogniged by the kernel.
I'm still investigating this issue, but drivers/nvdimm/of_pmem.c in linux
will be one of the solution to recognize RAM disk from kernel.
(In EDK2, the equivalent solution is called ACPI NFIT.)

On QEMU, I can not make DNS work from the QEMU guest.
The following commands work on qemu_arm64(manually set the http server ip in URI).
  => dhcp
  => setenv gatewayip 10.0.2.2
  => setenv httpserverip 134.160.38.1
  => efidebug boot add -u 3 debian-netinst http://134.160.38.1/Linux/debian/debian-cd/12.1.0/arm64/iso-cd/debian-12.1.0-arm64-netinst.iso
  => efidebug boot order 3
  => bootefi bootmgr

Masahisa Kojima (2):
  cmd: efidebug: add uri device path
  efi_loader: support boot from URI device path

 cmd/efidebug.c               |  39 +++++++
 lib/efi_loader/efi_bootmgr.c | 213 +++++++++++++++++++++++++++++++++++
 2 files changed, 252 insertions(+)


base-commit: 4e73b0153cce064e10dee4722cb2b6833361c227
-- 
2.34.1

[PATCH 1/2] cmd: efidebug: add uri device path

[Masahisa Kojima]

This adds the URI device path option for 'boot add' subcommand.
User can add the URI load option for downloading ISO image file
or EFI application through network(e.g. HTTP).

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 0be3af3e76..62f867df2a 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
 			argc -= 1;
 			argv += 1;
 			break;
+		case 'u':
+		{
+			char *pos;
+			int uridp_len;
+			struct efi_device_path_uri *uridp;
+
+			if (argc <  3 || lo.label) {
+				r = CMD_RET_USAGE;
+				goto out;
+			}
+			id = (int)hextoul(argv[1], &endp);
+			if (*endp != '\0' || id > 0xffff)
+				return CMD_RET_USAGE;
+
+			efi_create_indexed_name(var_name16, sizeof(var_name16),
+						"Boot", id);
+
+			label = efi_convert_string(argv[2]);
+			if (!label)
+				return CMD_RET_FAILURE;
+			lo.label = label;
+
+			uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1;
+			fp_free = efi_alloc(uridp_len + sizeof(END));
+			uridp = (struct efi_device_path_uri *)fp_free;
+			uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+			uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
+			uridp->dp.length = uridp_len;
+			strcpy(uridp->uri, argv[3]);
+			pos = (char *)uridp + uridp_len;
+			memcpy(pos, &END, sizeof(END));
+			fp_size += uridp_len + sizeof(END);
+			file_path = (struct efi_device_path *)uridp;
+			argc -= 3;
+			argv += 3;
+			break;
+		}
+
 		default:
 			r = CMD_RET_USAGE;
 			goto out;
@@ -1492,6 +1530,7 @@ static char efidebug_help_text[] =
 	"  -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n"
 	"  -i|-I <interface> <devnum>[:<part>] <initrd file path>\n"
 	"  (-b, -i for short form device path)\n"
+	"  -u <bootid> <label> <uri>\n"
 	"  -s '<optional data>'\n"
 	"efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
 	"  - delete UEFI BootXXXX variables\n"
-- 
2.34.1

[PATCH 2/2] efi_loader: support boot from URI device path

[Masahisa Kojima]

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. */
+	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)
+	{
+		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;
+		}
+	}
+
+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);
+	} 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

Re: [PATCH 2/2] efi_loader: support boot from URI device path

[Simon Glass]

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

Re: [PATCH 1/2] cmd: efidebug: add uri device path

[AKASHI Takahiro]

Hi Kojima-san,

On Wed, Aug 23, 2023 at 05:37:19PM +0900, Masahisa Kojima wrote:
> This adds the URI device path option for 'boot add' subcommand.
> User can add the URI load option for downloading ISO image file
> or EFI application through network(e.g. HTTP).
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 0be3af3e76..62f867df2a 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>  			argc -= 1;
>  			argv += 1;
>  			break;
> +		case 'u':
> +		{
> +			char *pos;
> +			int uridp_len;
> +			struct efi_device_path_uri *uridp;
> +
> +			if (argc <  3 || lo.label) {
> +				r = CMD_RET_USAGE;
> +				goto out;
> +			}
> +			id = (int)hextoul(argv[1], &endp);
> +			if (*endp != '\0' || id > 0xffff)
> +				return CMD_RET_USAGE;
> +
> +			efi_create_indexed_name(var_name16, sizeof(var_name16),
> +						"Boot", id);
> +
> +			label = efi_convert_string(argv[2]);
> +			if (!label)
> +				return CMD_RET_FAILURE;
> +			lo.label = label;
> +
> +			uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1;
> +			fp_free = efi_alloc(uridp_len + sizeof(END));
> +			uridp = (struct efi_device_path_uri *)fp_free;
> +			uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> +			uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
> +			uridp->dp.length = uridp_len;
> +			strcpy(uridp->uri, argv[3]);
> +			pos = (char *)uridp + uridp_len;
> +			memcpy(pos, &END, sizeof(END));
> +			fp_size += uridp_len + sizeof(END);
> +			file_path = (struct efi_device_path *)uridp;
> +			argc -= 3;
> +			argv += 3;
> +			break;
> +		}
> +
>  		default:
>  			r = CMD_RET_USAGE;
>  			goto out;
> @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] =
>  	"  -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n"
>  	"  -i|-I <interface> <devnum>[:<part>] <initrd file path>\n"
>  	"  (-b, -i for short form device path)\n"
> +	"  -u <bootid> <label> <uri>\n"

It might be a matter of personal preference, but
since the current syntax of efidebug is already much complex, 
I don't want to add more options unless it's necessary.
How about re-using "-B" option, say
   => efidebug -B 3 debian-netinst uri - https://foo.com/baa

BTW, I think that <bootid> and <label> should have been moved out of "-b|B"
when Ilias introduced this new syntax.

-Takahiro Akashi


>  	"  -s '<optional data>'\n"
>  	"efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
>  	"  - delete UEFI BootXXXX variables\n"
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] efi_loader: support boot from URI device path

[AKASHI Takahiro]

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
> 

Re: [PATCH 2/2] efi_loader: support boot from URI device path

[AKASHI Takahiro]

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

Re: [PATCH 1/2] cmd: efidebug: add uri device path

[Heinrich Schuchardt]

On 8/23/23 10:37, Masahisa Kojima wrote:
> This adds the URI device path option for 'boot add' subcommand.
> User can add the URI load option for downloading ISO image file
> or EFI application through network(e.g. HTTP).
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>   cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++
>   1 file changed, 39 insertions(+)
>
> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> index 0be3af3e76..62f867df2a 100644
> --- a/cmd/efidebug.c
> +++ b/cmd/efidebug.c
> @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>   			argc -= 1;
>   			argv += 1;
>   			break;
> +		case 'u':
> +		{
> +			char *pos;
> +			int uridp_len;
> +			struct efi_device_path_uri *uridp;
> +
> +			if (argc <  3 || lo.label) {
> +				r = CMD_RET_USAGE;
> +				goto out;
> +			}
> +			id = (int)hextoul(argv[1], &endp);
> +			if (*endp != '\0' || id > 0xffff)
> +				return CMD_RET_USAGE;
> +
> +			efi_create_indexed_name(var_name16, sizeof(var_name16),
> +						"Boot", id);
> +
> +			label = efi_convert_string(argv[2]);
> +			if (!label)
> +				return CMD_RET_FAILURE;
> +			lo.label = label;
> +
> +			uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; > +			fp_free = efi_alloc(uridp_len + sizeof(END));
> +			uridp = (struct efi_device_path_uri *)fp_free;
> +			uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> +			uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
> +			uridp->dp.length = uridp_len;
> +			strcpy(uridp->uri, argv[3]);

This assumes that argv[3] is a valid URI.

Would it be preferable to validate that the string is percent encoded,
conforms to RFC 3986, and contains a supported scheme, an authority, and
a path?

As a user I would like related errors to be caught at entry and not at
runtime.

Best regards

Heinrich

> +			pos = (char *)uridp + uridp_len;
> +			memcpy(pos, &END, sizeof(END));
> +			fp_size += uridp_len + sizeof(END);
> +			file_path = (struct efi_device_path *)uridp;
> +			argc -= 3;
> +			argv += 3;
> +			break;
> +		}
> +
>   		default:
>   			r = CMD_RET_USAGE;
>   			goto out;
> @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] =
>   	"  -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n"
>   	"  -i|-I <interface> <devnum>[:<part>] <initrd file path>\n"
>   	"  (-b, -i for short form device path)\n"
> +	"  -u <bootid> <label> <uri>\n"
>   	"  -s '<optional data>'\n"
>   	"efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
>   	"  - delete UEFI BootXXXX variables\n"

Re: [PATCH 2/2] efi_loader: support boot from URI device path

[Heinrich Schuchardt]

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,

Re: [PATCH 1/2] cmd: efidebug: add uri device path

[Masahisa Kojima]

On Thu, 24 Aug 2023 at 09:11, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Hi Kojima-san,
>
> On Wed, Aug 23, 2023 at 05:37:19PM +0900, Masahisa Kojima wrote:
> > This adds the URI device path option for 'boot add' subcommand.
> > User can add the URI load option for downloading ISO image file
> > or EFI application through network(e.g. HTTP).
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >  cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >
> > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > index 0be3af3e76..62f867df2a 100644
> > --- a/cmd/efidebug.c
> > +++ b/cmd/efidebug.c
> > @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
> >                       argc -= 1;
> >                       argv += 1;
> >                       break;
> > +             case 'u':
> > +             {
> > +                     char *pos;
> > +                     int uridp_len;
> > +                     struct efi_device_path_uri *uridp;
> > +
> > +                     if (argc <  3 || lo.label) {
> > +                             r = CMD_RET_USAGE;
> > +                             goto out;
> > +                     }
> > +                     id = (int)hextoul(argv[1], &endp);
> > +                     if (*endp != '\0' || id > 0xffff)
> > +                             return CMD_RET_USAGE;
> > +
> > +                     efi_create_indexed_name(var_name16, sizeof(var_name16),
> > +                                             "Boot", id);
> > +
> > +                     label = efi_convert_string(argv[2]);
> > +                     if (!label)
> > +                             return CMD_RET_FAILURE;
> > +                     lo.label = label;
> > +
> > +                     uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1;
> > +                     fp_free = efi_alloc(uridp_len + sizeof(END));
> > +                     uridp = (struct efi_device_path_uri *)fp_free;
> > +                     uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> > +                     uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
> > +                     uridp->dp.length = uridp_len;
> > +                     strcpy(uridp->uri, argv[3]);
> > +                     pos = (char *)uridp + uridp_len;
> > +                     memcpy(pos, &END, sizeof(END));
> > +                     fp_size += uridp_len + sizeof(END);
> > +                     file_path = (struct efi_device_path *)uridp;
> > +                     argc -= 3;
> > +                     argv += 3;
> > +                     break;
> > +             }
> > +
> >               default:
> >                       r = CMD_RET_USAGE;
> >                       goto out;
> > @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] =
> >       "  -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n"
> >       "  -i|-I <interface> <devnum>[:<part>] <initrd file path>\n"
> >       "  (-b, -i for short form device path)\n"
> > +     "  -u <bootid> <label> <uri>\n"
>
> It might be a matter of personal preference, but
> since the current syntax of efidebug is already much complex,
> I don't want to add more options unless it's necessary.
> How about re-using "-B" option, say
>    => efidebug -B 3 debian-netinst uri - https://foo.com/baa

I understand your concern. OK, I will add uri in -b|B and also update
documentation.

Thanks,
Masahisa Kojima

>
> BTW, I think that <bootid> and <label> should have been moved out of "-b|B"
> when Ilias introduced this new syntax.
>
> -Takahiro Akashi
>
>
> >       "  -s '<optional data>'\n"
> >       "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
> >       "  - delete UEFI BootXXXX variables\n"
> > --
> > 2.34.1
> >

Re: [PATCH 1/2] cmd: efidebug: add uri device path

[Masahisa Kojima]

On Thu, 24 Aug 2023 at 14:37, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 8/23/23 10:37, Masahisa Kojima wrote:
> > This adds the URI device path option for 'boot add' subcommand.
> > User can add the URI load option for downloading ISO image file
> > or EFI application through network(e.g. HTTP).
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >   cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 39 insertions(+)
> >
> > diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> > index 0be3af3e76..62f867df2a 100644
> > --- a/cmd/efidebug.c
> > +++ b/cmd/efidebug.c
> > @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
> >                       argc -= 1;
> >                       argv += 1;
> >                       break;
> > +             case 'u':
> > +             {
> > +                     char *pos;
> > +                     int uridp_len;
> > +                     struct efi_device_path_uri *uridp;
> > +
> > +                     if (argc <  3 || lo.label) {
> > +                             r = CMD_RET_USAGE;
> > +                             goto out;
> > +                     }
> > +                     id = (int)hextoul(argv[1], &endp);
> > +                     if (*endp != '\0' || id > 0xffff)
> > +                             return CMD_RET_USAGE;
> > +
> > +                     efi_create_indexed_name(var_name16, sizeof(var_name16),
> > +                                             "Boot", id);
> > +
> > +                     label = efi_convert_string(argv[2]);
> > +                     if (!label)
> > +                             return CMD_RET_FAILURE;
> > +                     lo.label = label;
> > +
> > +                     uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; > +                   fp_free = efi_alloc(uridp_len + sizeof(END));
> > +                     uridp = (struct efi_device_path_uri *)fp_free;
> > +                     uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> > +                     uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
> > +                     uridp->dp.length = uridp_len;
> > +                     strcpy(uridp->uri, argv[3]);
>
> This assumes that argv[3] is a valid URI.
>
> Would it be preferable to validate that the string is percent encoded,
> conforms to RFC 3986, and contains a supported scheme, an authority, and
> a path?
>
> As a user I would like related errors to be caught at entry and not at
> runtime.

OK, I agree. I will add input uri validation here.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> > +                     pos = (char *)uridp + uridp_len;
> > +                     memcpy(pos, &END, sizeof(END));
> > +                     fp_size += uridp_len + sizeof(END);
> > +                     file_path = (struct efi_device_path *)uridp;
> > +                     argc -= 3;
> > +                     argv += 3;
> > +                     break;
> > +             }
> > +
> >               default:
> >                       r = CMD_RET_USAGE;
> >                       goto out;
> > @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] =
> >       "  -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n"
> >       "  -i|-I <interface> <devnum>[:<part>] <initrd file path>\n"
> >       "  (-b, -i for short form device path)\n"
> > +     "  -u <bootid> <label> <uri>\n"
> >       "  -s '<optional data>'\n"
> >       "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
> >       "  - delete UEFI BootXXXX variables\n"
>

Re: [PATCH 2/2] efi_loader: support boot from URI device path

[Masahisa Kojima]

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

Re: [PATCH 2/2] efi_loader: support boot from URI device path

[Masahisa Kojima]

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

Re: [PATCH 2/2] efi_loader: support boot from URI device path

[Masahisa Kojima]

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

Re: [PATCH 1/2] cmd: efidebug: add uri device path

[Heinrich Schuchardt]

On 8/25/23 07:57, Masahisa Kojima wrote:
> On Thu, 24 Aug 2023 at 09:11, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
>>
>> Hi Kojima-san,
>>
>> On Wed, Aug 23, 2023 at 05:37:19PM +0900, Masahisa Kojima wrote:
>>> This adds the URI device path option for 'boot add' subcommand.
>>> User can add the URI load option for downloading ISO image file
>>> or EFI application through network(e.g. HTTP).
>>>
>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>> ---
>>>   cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 39 insertions(+)
>>>
>>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
>>> index 0be3af3e76..62f867df2a 100644
>>> --- a/cmd/efidebug.c
>>> +++ b/cmd/efidebug.c
>>> @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
>>>                        argc -= 1;
>>>                        argv += 1;
>>>                        break;
>>> +             case 'u':
>>> +             {
>>> +                     char *pos;
>>> +                     int uridp_len;
>>> +                     struct efi_device_path_uri *uridp;
>>> +
>>> +                     if (argc <  3 || lo.label) {
>>> +                             r = CMD_RET_USAGE;
>>> +                             goto out;
>>> +                     }
>>> +                     id = (int)hextoul(argv[1], &endp);
>>> +                     if (*endp != '\0' || id > 0xffff)
>>> +                             return CMD_RET_USAGE;
>>> +
>>> +                     efi_create_indexed_name(var_name16, sizeof(var_name16),
>>> +                                             "Boot", id);
>>> +
>>> +                     label = efi_convert_string(argv[2]);
>>> +                     if (!label)
>>> +                             return CMD_RET_FAILURE;
>>> +                     lo.label = label;
>>> +
>>> +                     uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1;
>>> +                     fp_free = efi_alloc(uridp_len + sizeof(END));
>>> +                     uridp = (struct efi_device_path_uri *)fp_free;
>>> +                     uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
>>> +                     uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
>>> +                     uridp->dp.length = uridp_len;
>>> +                     strcpy(uridp->uri, argv[3]);
>>> +                     pos = (char *)uridp + uridp_len;
>>> +                     memcpy(pos, &END, sizeof(END));
>>> +                     fp_size += uridp_len + sizeof(END);
>>> +                     file_path = (struct efi_device_path *)uridp;
>>> +                     argc -= 3;
>>> +                     argv += 3;
>>> +                     break;
>>> +             }
>>> +
>>>                default:
>>>                        r = CMD_RET_USAGE;
>>>                        goto out;
>>> @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] =
>>>        "  -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n"
>>>        "  -i|-I <interface> <devnum>[:<part>] <initrd file path>\n"
>>>        "  (-b, -i for short form device path)\n"
>>> +     "  -u <bootid> <label> <uri>\n"
>>
>> It might be a matter of personal preference, but
>> since the current syntax of efidebug is already much complex,
>> I don't want to add more options unless it's necessary.
>> How about re-using "-B" option, say
>>     => efidebug -B 3 debian-netinst uri - https://foo.com/baa

You just replace one set of alternatives (-B -b -u) against another
(mmc, nvme, uri, ...). I cannot see that this reduces complexity.

>
> I understand your concern. OK, I will add uri in -b|B and also update
> documentation.

It is unclear to me what the difference between -b and -B shall be for
URIs. Would -B be with the device path of the network device
(/VenHw()/MAC()/URI()) and -b without (/URI())?

Best regards

Heinrich

>
> Thanks,
> Masahisa Kojima
>
>>
>> BTW, I think that <bootid> and <label> should have been moved out of "-b|B"
>> when Ilias introduced this new syntax.
>>
>> -Takahiro Akashi
>>
>>
>>>        "  -s '<optional data>'\n"
>>>        "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
>>>        "  - delete UEFI BootXXXX variables\n"
>>> --
>>> 2.34.1
>>>

Re: [PATCH 1/2] cmd: efidebug: add uri device path

[Masahisa Kojima]

Hi Heinrich,

On Sat, 26 Aug 2023 at 11:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 8/25/23 07:57, Masahisa Kojima wrote:
> > On Thu, 24 Aug 2023 at 09:11, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> >>
> >> Hi Kojima-san,
> >>
> >> On Wed, Aug 23, 2023 at 05:37:19PM +0900, Masahisa Kojima wrote:
> >>> This adds the URI device path option for 'boot add' subcommand.
> >>> User can add the URI load option for downloading ISO image file
> >>> or EFI application through network(e.g. HTTP).
> >>>
> >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>> ---
> >>>   cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++
> >>>   1 file changed, 39 insertions(+)
> >>>
> >>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c
> >>> index 0be3af3e76..62f867df2a 100644
> >>> --- a/cmd/efidebug.c
> >>> +++ b/cmd/efidebug.c
> >>> @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
> >>>                        argc -= 1;
> >>>                        argv += 1;
> >>>                        break;
> >>> +             case 'u':
> >>> +             {
> >>> +                     char *pos;
> >>> +                     int uridp_len;
> >>> +                     struct efi_device_path_uri *uridp;
> >>> +
> >>> +                     if (argc <  3 || lo.label) {
> >>> +                             r = CMD_RET_USAGE;
> >>> +                             goto out;
> >>> +                     }
> >>> +                     id = (int)hextoul(argv[1], &endp);
> >>> +                     if (*endp != '\0' || id > 0xffff)
> >>> +                             return CMD_RET_USAGE;
> >>> +
> >>> +                     efi_create_indexed_name(var_name16, sizeof(var_name16),
> >>> +                                             "Boot", id);
> >>> +
> >>> +                     label = efi_convert_string(argv[2]);
> >>> +                     if (!label)
> >>> +                             return CMD_RET_FAILURE;
> >>> +                     lo.label = label;
> >>> +
> >>> +                     uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1;
> >>> +                     fp_free = efi_alloc(uridp_len + sizeof(END));
> >>> +                     uridp = (struct efi_device_path_uri *)fp_free;
> >>> +                     uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> >>> +                     uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
> >>> +                     uridp->dp.length = uridp_len;
> >>> +                     strcpy(uridp->uri, argv[3]);
> >>> +                     pos = (char *)uridp + uridp_len;
> >>> +                     memcpy(pos, &END, sizeof(END));
> >>> +                     fp_size += uridp_len + sizeof(END);
> >>> +                     file_path = (struct efi_device_path *)uridp;
> >>> +                     argc -= 3;
> >>> +                     argv += 3;
> >>> +                     break;
> >>> +             }
> >>> +
> >>>                default:
> >>>                        r = CMD_RET_USAGE;
> >>>                        goto out;
> >>> @@ -1492,6 +1530,7 @@ static char efidebug_help_text[] =
> >>>        "  -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n"
> >>>        "  -i|-I <interface> <devnum>[:<part>] <initrd file path>\n"
> >>>        "  (-b, -i for short form device path)\n"
> >>> +     "  -u <bootid> <label> <uri>\n"
> >>
> >> It might be a matter of personal preference, but
> >> since the current syntax of efidebug is already much complex,
> >> I don't want to add more options unless it's necessary.
> >> How about re-using "-B" option, say
> >>     => efidebug -B 3 debian-netinst uri - https://foo.com/baa
>
> You just replace one set of alternatives (-B -b -u) against another
> (mmc, nvme, uri, ...). I cannot see that this reduces complexity.
>
> >
> > I understand your concern. OK, I will add uri in -b|B and also update
> > documentation.
>
> It is unclear to me what the difference between -b and -B shall be for
> URIs. Would -B be with the device path of the network device
> (/VenHw()/MAC()/URI()) and -b without (/URI())?

Differentiating -b and -B for URI is not mandatory, I think.
To keep implementation simple, I would like to add '-u' option as the
original patch does.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> > Thanks,
> > Masahisa Kojima
> >
> >>
> >> BTW, I think that <bootid> and <label> should have been moved out of "-b|B"
> >> when Ilias introduced this new syntax.
> >>
> >> -Takahiro Akashi
> >>
> >>
> >>>        "  -s '<optional data>'\n"
> >>>        "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
> >>>        "  - delete UEFI BootXXXX variables\n"
> >>> --
> >>> 2.34.1
> >>>
>