[PATCH v2 0/6] Add EFI HTTP boot support

[PATCH v2 0/6] 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).

This version still does not include the test.

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

[TODO]
- add test
- stricter wget uri check
- omit the dns process if the given uri has ip address
   -> this will be supported when the lwip migration completes

[change log]
v1 -> v2
- carve out the network handling(wget and dns code) under net/wget.c
- carve out ramdisk creation code under drivers/block/blkmap_helper.c
- wget supports the valid range check to store the received blocks using lmb
- support when the downloaded image have no partiton table but a file system
- not start the .efi file in try_load_entry()
- call efi_check_pe() for .efi file to check the file is PE-COFF image
- add documentation for EFI HTTP Boot

Masahisa Kojima (6):
  net: wget: prevent overwriting reserved memory
  net: wget: add wget with dns utility function
  blk: blkmap: add ramdisk creation utility function
  efi_loader: support boot from URI device path
  cmd: efidebug: add uri device path
  doc: uefi: add HTTP Boot support

 cmd/efidebug.c                |  50 +++++++++
 doc/develop/uefi/uefi.rst     |  21 ++++
 drivers/block/Makefile        |   1 +
 drivers/block/blkmap.c        |  15 ---
 drivers/block/blkmap_helper.c |  53 +++++++++
 include/blkmap.h              |  29 +++++
 include/net.h                 |   8 ++
 lib/efi_loader/efi_bootmgr.c  | 197 +++++++++++++++++++++++++++++++++
 net/wget.c                    | 202 ++++++++++++++++++++++++++++++++--
 9 files changed, 554 insertions(+), 22 deletions(-)
 create mode 100644 drivers/block/blkmap_helper.c


base-commit: cc889bd0754e50a3cd50e8ed3094388480fbec86
-- 
2.34.1

[PATCH v2 1/6] net: wget: prevent overwriting reserved memory

[Masahisa Kojima]

This introduces the valid range check to store the received
blocks using lmb. The same logic is implemented in tftp.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 net/wget.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 7 deletions(-)

diff --git a/net/wget.c b/net/wget.c
index 2dbfeb1a1d..8bf4db4d04 100644
--- a/net/wget.c
+++ b/net/wget.c
@@ -4,16 +4,20 @@
  * Copyright Duncan Hare <dh@synoia.com> 2017
  */
 
+#include <asm/global_data.h>
 #include <command.h>
 #include <common.h>
 #include <display_options.h>
 #include <env.h>
 #include <image.h>
+#include <lmb.h>
 #include <mapmem.h>
 #include <net.h>
 #include <net/tcp.h>
 #include <net/wget.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 static const char bootfile1[] = "GET ";
 static const char bootfile3[] = " HTTP/1.0\r\n\r\n";
 static const char http_eom[] = "\r\n\r\n";
@@ -55,6 +59,32 @@ static unsigned int retry_tcp_ack_num;	/* TCP retry acknowledge number*/
 static unsigned int retry_tcp_seq_num;	/* TCP retry sequence number */
 static int retry_len;			/* TCP retry length */
 
+#ifdef CONFIG_LMB
+static ulong wget_load_size;
+#endif
+
+/**
+ * wget_init_max_size() - initialize maximum load size
+ *
+ * Return:	0 if success, -1 if fails
+ */
+static int wget_init_load_size(void)
+{
+#ifdef CONFIG_LMB
+	struct lmb lmb;
+	phys_size_t max_size;
+
+	lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
+
+	max_size = lmb_get_free_size(&lmb, image_load_addr);
+	if (!max_size)
+		return -1;
+
+	wget_load_size = max_size;
+#endif
+	return 0;
+}
+
 /**
  * store_block() - store block in memory
  * @src: source of data
@@ -63,10 +93,24 @@ static int retry_len;			/* TCP retry length */
  */
 static inline int store_block(uchar *src, unsigned int offset, unsigned int len)
 {
+	ulong store_addr = image_load_addr + offset;
 	ulong newsize = offset + len;
 	uchar *ptr;
 
-	ptr = map_sysmem(image_load_addr + offset, len);
+#ifdef CONFIG_LMB
+	ulong end_addr = image_load_addr + wget_load_size;
+
+	if (!end_addr)
+		end_addr = ULONG_MAX;
+
+	if (store_addr < image_load_addr ||
+		store_addr + len > end_addr) {
+		puts("\nwget error: ");
+		puts("trying to overwrite reserved memory...\n");
+		return -1;
+	}
+#endif
+	ptr = map_sysmem(store_addr, len);
 	memcpy(ptr, src, len);
 	unmap_sysmem(ptr);
 
@@ -240,25 +284,37 @@ static void wget_connected(uchar *pkt, unsigned int tcp_seq_num,
 
 			net_boot_file_size = 0;
 
-			if (len > hlen)
-				store_block(pkt + hlen, 0, len - hlen);
+			if (len > hlen) {
+				if (store_block(pkt + hlen, 0, len - hlen) != 0) {
+					wget_loop_state = NETLOOP_FAIL;
+					wget_fail("wget: store error\n", tcp_seq_num, tcp_ack_num, action);
+					return;
+				}
+			}
 
 			debug_cond(DEBUG_WGET,
 				   "wget: Connected Pkt %p hlen %x\n",
 				   pkt, hlen);
 
 			for (i = 0; i < pkt_q_idx; i++) {
+				int err;
+
 				ptr1 = map_sysmem(
 					(phys_addr_t)(pkt_q[i].pkt),
 					pkt_q[i].len);
-				store_block(ptr1,
-					    pkt_q[i].tcp_seq_num -
-					    initial_data_seq_num,
-					    pkt_q[i].len);
+				err = store_block(ptr1,
+						  pkt_q[i].tcp_seq_num -
+						  initial_data_seq_num,
+						  pkt_q[i].len);
 				unmap_sysmem(ptr1);
 				debug_cond(DEBUG_WGET,
 					   "wget: Connctd pkt Q %p len %x\n",
 					   pkt_q[i].pkt, pkt_q[i].len);
+				if (err) {
+					wget_loop_state = NETLOOP_FAIL;
+					wget_fail("wget: store error\n", tcp_seq_num, tcp_ack_num, action);
+					return;
+				}
 			}
 		}
 	}
@@ -420,6 +476,12 @@ void wget_start(void)
 	debug_cond(DEBUG_WGET,
 		   "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
 
+	if (wget_init_load_size()) {
+		puts("\nwget error: ");
+		puts("trying to overwrite reserved memory...\n");
+		return;
+	}
+
 	net_set_timeout_handler(wget_timeout, wget_timeout_handler);
 	tcp_set_tcp_handler(wget_handler);
 
-- 
2.34.1

[PATCH v2 2/6] net: wget: add wget with dns utility function

[Masahisa Kojima]

Current wget takes the target uri in this format:
 "<http server ip>:<file path>"  e.g.) 192.168.1.1:/bar
The http server ip address must be resolved before
calling wget.

This commit adds the utility function runs wget with dhs.
User can call wget with the uri like "http://foo/bar".

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 include/net.h |  4 ++++
 net/wget.c    | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/include/net.h b/include/net.h
index e254df7d7f..b4bbf95732 100644
--- a/include/net.h
+++ b/include/net.h
@@ -926,4 +926,8 @@ void eth_set_enable_bootdevs(bool enable);
 static inline void eth_set_enable_bootdevs(bool enable) {}
 #endif
 
+#if (IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
+int wget_with_dns(ulong dst_addr, char *uri);
+#endif
+
 #endif /* __NET_H__ */
diff --git a/net/wget.c b/net/wget.c
index 8bf4db4d04..5718f1cf92 100644
--- a/net/wget.c
+++ b/net/wget.c
@@ -15,6 +15,7 @@
 #include <net.h>
 #include <net/tcp.h>
 #include <net/wget.h>
+#include <stdlib.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -500,3 +501,56 @@ void wget_start(void)
 
 	wget_send(TCP_SYN, 0, 0, 0);
 }
+
+#if (IS_ENABLED(CONFIG_CMD_DNS))
+int wget_with_dns(ulong dst_addr, char *uri)
+{
+	int ret;
+	char *s, *host_name, *file_name, *str_copy;
+
+	/*
+	 * Download file using wget.
+	 *
+	 * 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.
+	 */
+	str_copy = strdup(uri);
+	if (!str_copy)
+		return -ENOMEM;
+
+	s = str_copy + strlen("http://");
+	host_name = strsep(&s, "/");
+	if (!s) {
+		log_err("Error: invalied uri, no file path\n");
+		ret = -EINVAL;
+		goto out;
+	}
+	file_name = s;
+
+	/* TODO: If the given uri has ip address for the http server, skip dns */
+	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 = -EINVAL;
+		goto out;
+	}
+	s = env_get("httpserverip");
+	if (!s) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	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);
+	image_load_addr = dst_addr;
+	ret = net_loop(WGET);
+
+out:
+	free(str_copy);
+
+	return ret;
+}
+#endif
-- 
2.34.1

[PATCH v2 3/6] blk: blkmap: add ramdisk creation utility function

[Masahisa Kojima]

User needs to call several functions to create the ramdisk
with blkmap.
This adds the utility function to create blkmap device and
mount the ramdisk.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 drivers/block/Makefile        |  1 +
 drivers/block/blkmap.c        | 15 ----------
 drivers/block/blkmap_helper.c | 53 +++++++++++++++++++++++++++++++++++
 include/blkmap.h              | 29 +++++++++++++++++++
 4 files changed, 83 insertions(+), 15 deletions(-)
 create mode 100644 drivers/block/blkmap_helper.c

diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index a161d145fd..c3ccfc03e5 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -15,6 +15,7 @@ endif
 obj-$(CONFIG_SANDBOX) += sandbox.o host-uclass.o host_dev.o
 obj-$(CONFIG_$(SPL_TPL_)BLOCK_CACHE) += blkcache.o
 obj-$(CONFIG_BLKMAP) += blkmap.o
+obj-$(CONFIG_BLKMAP) += blkmap_helper.o
 
 obj-$(CONFIG_EFI_MEDIA) += efi-media-uclass.o
 obj-$(CONFIG_EFI_MEDIA_SANDBOX) += sb_efi_media.o
diff --git a/drivers/block/blkmap.c b/drivers/block/blkmap.c
index 2bb0acc20f..4e95997f61 100644
--- a/drivers/block/blkmap.c
+++ b/drivers/block/blkmap.c
@@ -66,21 +66,6 @@ struct blkmap_slice {
 	void (*destroy)(struct blkmap *bm, struct blkmap_slice *bms);
 };
 
-/**
- * struct blkmap - Block map
- *
- * Data associated with a blkmap.
- *
- * @label: Human readable name of this blkmap
- * @blk: Underlying block device
- * @slices: List of slices associated with this blkmap
- */
-struct blkmap {
-	char *label;
-	struct udevice *blk;
-	struct list_head slices;
-};
-
 static bool blkmap_slice_contains(struct blkmap_slice *bms, lbaint_t blknr)
 {
 	return (blknr >= bms->blknr) && (blknr < (bms->blknr + bms->blkcnt));
diff --git a/drivers/block/blkmap_helper.c b/drivers/block/blkmap_helper.c
new file mode 100644
index 0000000000..0f80035f57
--- /dev/null
+++ b/drivers/block/blkmap_helper.c
@@ -0,0 +1,53 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * blkmap helper function
+ *
+ * Copyright (c) 2023, Linaro Limited
+ */
+
+#include <blk.h>
+#include <blkmap.h>
+#include <dm/device.h>
+#include <dm/device-internal.h>
+
+int blkmap_create_ramdisk(const char *label, ulong image_addr, int image_size,
+			  struct udevice **devp)
+{
+	int ret;
+	lbaint_t blknum;
+	struct blkmap *bm;
+	struct blk_desc *desc;
+	struct udevice *bm_dev;
+
+	ret = blkmap_create(label, &bm_dev);
+	if (ret) {
+		log_err("failed to create blkmap\n");
+		return ret;
+	}
+
+	bm = dev_get_plat(bm_dev);
+	desc = dev_get_uclass_plat(bm->blk);
+	blknum = image_size >> desc->log2blksz;
+	ret = blkmap_map_pmem(bm_dev, 0, blknum, image_addr);
+	if (ret) {
+		log_err("Unable to map %#llx at block %d : %d\n",
+			(unsigned long long)image_addr, 0, ret);
+		goto err;
+	}
+	log_info("Block %d+0x" LBAF " mapped to %#llx\n", 0, blknum,
+		 (unsigned long long)image_addr);
+
+	ret = device_probe(bm->blk);
+	if (ret)
+		goto err;
+
+	if (devp)
+		*devp = bm_dev;
+
+	return 0;
+
+err:
+	blkmap_destroy(bm_dev);
+
+	return ret;
+}
diff --git a/include/blkmap.h b/include/blkmap.h
index af54583c7d..0d87e6db6b 100644
--- a/include/blkmap.h
+++ b/include/blkmap.h
@@ -7,6 +7,23 @@
 #ifndef _BLKMAP_H
 #define _BLKMAP_H
 
+#include <dm/lists.h>
+
+/**
+ * struct blkmap - Block map
+ *
+ * Data associated with a blkmap.
+ *
+ * @label: Human readable name of this blkmap
+ * @blk: Underlying block device
+ * @slices: List of slices associated with this blkmap
+ */
+struct blkmap {
+	char *label;
+	struct udevice *blk;
+	struct list_head slices;
+};
+
 /**
  * blkmap_map_linear() - Map region of other block device
  *
@@ -74,4 +91,16 @@ int blkmap_create(const char *label, struct udevice **devp);
  */
 int blkmap_destroy(struct udevice *dev);
 
+/**
+ * blkmap_create_ramdisk() - Create new ramdisk with blkmap
+ *
+ * @label: Label of the new blkmap
+ * @image_addr: Target memory start address of this mapping
+ * @image_size: Target memory size of this mapping
+ * @devp: Updated with the address of the created blkmap device
+ * Returns: 0 on success, negative error code on failure
+ */
+int blkmap_create_ramdisk(const char *label, ulong image_addr, int image_size,
+			  struct udevice **devp);
+
 #endif	/* _BLKMAP_H */
-- 
2.34.1

[PATCH v2 4/6] 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 | 197 +++++++++++++++++++++++++++++++++++
 1 file changed, 197 insertions(+)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index a40762c74c..0e8d2ca9d1 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,193 @@ out:
 	return ret;
 }
 
+#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
+/**
+ * mount_image() - mount the image with blkmap
+ *
+ * @lo_label	u16 label string of load option
+ * @image_addr:	image address
+ * @image_size	image size
+ * Return:	pointer to the UCLASS_BLK udevice, NULL if failed
+ */
+static struct udevice *mount_image(u16 *lo_label, ulong image_addr, int image_size)
+{
+	int err;
+	struct blkmap *bm;
+	struct udevice *bm_dev;
+	char *label = NULL, *p;
+
+	label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
+	if (!label)
+		return NULL;
+
+	p = label;
+	utf16_utf8_strcpy(&p, lo_label);
+	err = blkmap_create_ramdisk(label, image_addr, image_size, &bm_dev);
+	if (err) {
+		efi_free_pool(label);
+		return NULL;
+	}
+	bm = dev_get_plat(bm_dev);
+
+	efi_free_pool(label);
+
+	return bm->blk;
+}
+
+/**
+ * try_load_default_file() - try to load the default file
+ *
+ * Search the device having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
+ * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
+ *
+ * @dev			pointer to the UCLASS_BLK or UCLASS_PARTITION udevice
+ * @image_handle:	pointer to handle for newly installed image
+ * Return:		status code
+ */
+static efi_status_t try_load_default_file(struct udevice *dev,
+					  efi_handle_t *image_handle)
+{
+	efi_status_t ret;
+	efi_handle_t bm_handle;
+	struct efi_handler *handler;
+	struct efi_device_path *file_path;
+	struct efi_device_path *device_path;
+
+	if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&bm_handle)) {
+		log_warning("DM_TAG_EFI not found\n");
+		return EFI_INVALID_PARAMETER;
+	}
+
+	ret = efi_search_protocol(bm_handle,
+				  &efi_simple_file_system_protocol_guid, &handler);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	ret = efi_search_protocol(bm_handle, &efi_guid_device_path, &handler);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	ret = efi_protocol_open(handler, (void **)&device_path, efi_root, NULL,
+				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	file_path = expand_media_path(device_path);
+	ret = EFI_CALL(efi_load_image(true, efi_root, file_path, NULL, 0,
+				      image_handle));
+
+	efi_free_pool(file_path);
+
+	return ret;
+}
+
+/**
+ * load_default_file_from_blk_dev() - load the default file
+ *
+ * @blk		pointer to the UCLASS_BLK udevice
+ * @handle:	pointer to handle for newly installed image
+ * Return:	status code
+ */
+static efi_status_t load_default_file_from_blk_dev(struct udevice *blk,
+						   efi_handle_t *handle)
+{
+	efi_status_t ret;
+	struct udevice *partition;
+
+	/* image that has no partition table but a file system */
+	ret = try_load_default_file(blk, handle);
+	if (ret == EFI_SUCCESS)
+		return ret;
+
+	/* try the partitions */
+	device_foreach_child(partition, blk) {
+		enum uclass_id id;
+
+		id = device_get_uclass_id(partition);
+		if (id != UCLASS_PARTITION)
+			continue;
+
+		ret = try_load_default_file(partition, handle);
+		if (ret == EFI_SUCCESS)
+			return ret;
+	}
+
+	return EFI_NOT_FOUND;
+}
+
+/**
+ * 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)
+{
+	char *s;
+	int uri_len;
+	int image_size;
+	efi_status_t ret;
+	ulong image_addr;
+
+	s = env_get("loadaddr");
+	if (!s) {
+		log_err("Error: loadaddr is not set\n");
+		return EFI_INVALID_PARAMETER;
+	}
+	image_addr = hextoul(s, NULL);
+	image_size = wget_with_dns(image_addr, uridp->uri);
+	if (image_size < 0)
+		return EFI_INVALID_PARAMETER;
+
+	/*
+	 * If the file extension is ".iso" or ".img", mount it and try to load
+	 * the default file.
+	 * If the file is ".efi" and PE-COFF image, load the downloaded file.
+	 */
+	uri_len = strlen(uridp->uri); /* todo: directly use uridp->uri */
+	if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
+	    !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
+		struct udevice *blk;
+
+		blk = mount_image(lo_label, image_addr, image_size);
+		if (!blk)
+			return EFI_INVALID_PARAMETER;
+
+		ret = load_default_file_from_blk_dev(blk, handle);
+	} else if (!strncmp(&uridp->uri[uri_len - 4], ".efi", 4)) {
+		efi_handle_t mem_handle = NULL;
+		struct efi_device_path *file_path = NULL;
+
+		ret = efi_check_pe((void *)image_addr, image_size, NULL);
+		if (ret != EFI_SUCCESS) {
+			log_err("Error: downloaded image is not a PE-COFF image\n");
+			return EFI_INVALID_PARAMETER;
+		}
+
+		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
+					    (uintptr_t)image_addr, image_size);
+		ret = efi_install_multiple_protocol_interfaces(
+			&mem_handle, &efi_guid_device_path, file_path, NULL);
+		if (ret != EFI_SUCCESS)
+			return EFI_INVALID_PARAMETER;
+
+		ret = EFI_CALL(efi_load_image(false, efi_root, file_path,
+					      (void *)image_addr, image_size,
+					      handle));
+	} else {
+		log_err("Error: file type is not supported\n");
+		return EFI_INVALID_PARAMETER;
+	}
+
+	return ret;
+}
+#endif
+
 /**
  * try_load_entry() - try to load image for boot option
  *
@@ -211,6 +402,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

[PATCH v2 5/6] 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. Currently HTTP is only supported.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 cmd/efidebug.c | 50 +++++++++++++++++++++++++++++++++++
 include/net.h  |  4 +++
 net/wget.c     | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 0be3af3e76..f2fd6ba71d 100644
--- a/cmd/efidebug.c
+++ b/cmd/efidebug.c
@@ -19,6 +19,7 @@
 #include <log.h>
 #include <malloc.h>
 #include <mapmem.h>
+#include <net.h>
 #include <part.h>
 #include <search.h>
 #include <linux/ctype.h>
@@ -829,6 +830,52 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag,
 			argc -= 1;
 			argv += 1;
 			break;
+#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
+		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;
+			if (!wget_validate_uri(argv[3])) {
+				printf("ERROR: invalid URI\n");
+				r = CMD_RET_FAILURE;
+				goto out;
+			}
+
+			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;
+		}
+#endif
+
 		default:
 			r = CMD_RET_USAGE;
 			goto out;
@@ -1492,6 +1539,9 @@ 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"
+#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
+	"  -u <bootid> <label> <uri>\n"
+#endif
 	"  -s '<optional data>'\n"
 	"efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
 	"  - delete UEFI BootXXXX variables\n"
diff --git a/include/net.h b/include/net.h
index b4bbf95732..ae9ba7544a 100644
--- a/include/net.h
+++ b/include/net.h
@@ -930,4 +930,8 @@ static inline void eth_set_enable_bootdevs(bool enable) {}
 int wget_with_dns(ulong dst_addr, char *uri);
 #endif
 
+#if (IS_ENABLED(CONFIG_CMD_WGET))
+bool wget_validate_uri(char *uri);
+#endif
+
 #endif /* __NET_H__ */
diff --git a/net/wget.c b/net/wget.c
index 5718f1cf92..717ef33695 100644
--- a/net/wget.c
+++ b/net/wget.c
@@ -554,3 +554,75 @@ out:
 	return ret;
 }
 #endif
+
+/**
+ * wget_validate_uri() - validate the uri for wget
+ *
+ * @uri:	uri string
+ * Return:	true on success, false on failure
+ */
+bool wget_validate_uri(char *uri)
+{
+	char c;
+	bool ret = true;
+	char *str_copy, *s, *authority;
+
+	/* TODO: strict uri conformance check */
+
+	/*
+	 * Uri is expected to be correctly percent encoded.
+	 * This is the minimum check, control codes(0x1-0x19, 0x7F, except '\0')
+	 * and space character(0x20) are not allowed.
+	 */
+	for (c = 0x1; c < 0x21; c++) {
+		if (strchr(uri, c)) {
+			printf("invalid character is used\n");
+			return false;
+		}
+	}
+	if (strchr(uri, 0x7f)) {
+		printf("invalid character is used\n");
+		return false;
+	}
+
+	/*
+	 * This follows the current U-Boot wget implementation.
+	 * scheme: only "http:" is supported
+	 * authority:
+	 *   - user information: not supported
+	 *   - host: supported
+	 *   - port: not supported(always use the default port)
+	 */
+	if (strncmp(uri, "http://", 7)) {
+		printf("only http:// is supported\n");
+		return false;
+	}
+	str_copy = strdup(uri);
+	if (!str_copy)
+		return false;
+
+	s = str_copy + strlen("http://");
+	authority = strsep(&s, "/");
+	if (!s) {
+		printf("invalid uri, no file path\n");
+		ret = false;
+		goto out;
+	}
+	s = strchr(authority, '@');
+	if (s) {
+		printf("user information is not supported\n");
+		ret = false;
+		goto out;
+	}
+	s = strchr(authority, ':');
+	if (s) {
+		printf("user defined port is not supported\n");
+		ret = false;
+		goto out;
+	}
+
+out:
+	free(str_copy);
+
+	return ret;
+}
-- 
2.34.1

[PATCH v2 6/6] doc: uefi: add HTTP Boot support

[Masahisa Kojima]

This adds the description about HTTP Boot.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 doc/develop/uefi/uefi.rst | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
index a7a41f2fac..fd87bb1edb 100644
--- a/doc/develop/uefi/uefi.rst
+++ b/doc/develop/uefi/uefi.rst
@@ -594,6 +594,27 @@ UEFI variables. Booting according to these variables is possible via::
 As of U-Boot v2020.10 UEFI variables cannot be set at runtime. The U-Boot
 command 'efidebug' can be used to set the variables.
 
+UEFI HTTP Boot
+~~~~~~~~~~~~~~
+
+HTTP Boot provides the capability for system deployment and configuration
+over the network. HTTP Boot can be activated by specifying::
+
+    CONFIG_CMD_DNS
+    CONFIG_CMD_WGET
+    CONFIG_BLKMAP
+
+Set up the load option specifying the target URI::
+
+    efidebug boot add -u 1 netinst http://foo/bar
+
+When this load option is selected as boot selection, resolve the
+host ip address by dhs, then download the file with wget.
+If the downloaded file extension is .iso or .img file, efibootmgr tries to
+mount the image and boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
+If the downloaded file extension is .efi and file is PE-COFF image,
+load the downloaded file and start it.
+
 Executing the built in hello world application
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.34.1

Re: [PATCH v2 1/6] net: wget: prevent overwriting reserved memory

[Simon Glass]

Hi Masahisa,

On Fri, 1 Sept 2023 at 04:26, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> This introduces the valid range check to store the received
> blocks using lmb. The same logic is implemented in tftp.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  net/wget.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 69 insertions(+), 7 deletions(-)
>
> diff --git a/net/wget.c b/net/wget.c
> index 2dbfeb1a1d..8bf4db4d04 100644
> --- a/net/wget.c
> +++ b/net/wget.c
> @@ -4,16 +4,20 @@
>   * Copyright Duncan Hare <dh@synoia.com> 2017
>   */
>
> +#include <asm/global_data.h>
>  #include <command.h>
>  #include <common.h>
>  #include <display_options.h>
>  #include <env.h>
>  #include <image.h>
> +#include <lmb.h>
>  #include <mapmem.h>
>  #include <net.h>
>  #include <net/tcp.h>
>  #include <net/wget.h>
>
> +DECLARE_GLOBAL_DATA_PTR;
> +
>  static const char bootfile1[] = "GET ";
>  static const char bootfile3[] = " HTTP/1.0\r\n\r\n";
>  static const char http_eom[] = "\r\n\r\n";
> @@ -55,6 +59,32 @@ static unsigned int retry_tcp_ack_num;       /* TCP retry acknowledge number*/
>  static unsigned int retry_tcp_seq_num; /* TCP retry sequence number */
>  static int retry_len;                  /* TCP retry length */
>
> +#ifdef CONFIG_LMB
> +static ulong wget_load_size;
> +#endif
> +
> +/**
> + * wget_init_max_size() - initialize maximum load size
> + *
> + * Return:     0 if success, -1 if fails
> + */
> +static int wget_init_load_size(void)
> +{
> +#ifdef CONFIG_LMB

please can you use if IS_ENABLED() throughout?

> +       struct lmb lmb;
> +       phys_size_t max_size;
> +
> +       lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
> +
> +       max_size = lmb_get_free_size(&lmb, image_load_addr);
> +       if (!max_size)
> +               return -1;
> +
> +       wget_load_size = max_size;
> +#endif
> +       return 0;
> +}
> +
>  /**
>   * store_block() - store block in memory
>   * @src: source of data
> @@ -63,10 +93,24 @@ static int retry_len;                       /* TCP retry length */
>   */
>  static inline int store_block(uchar *src, unsigned int offset, unsigned int len)
>  {
> +       ulong store_addr = image_load_addr + offset;
>         ulong newsize = offset + len;
>         uchar *ptr;
>
> -       ptr = map_sysmem(image_load_addr + offset, len);
> +#ifdef CONFIG_LMB
> +       ulong end_addr = image_load_addr + wget_load_size;
> +
> +       if (!end_addr)
> +               end_addr = ULONG_MAX;
> +
> +       if (store_addr < image_load_addr ||
> +               store_addr + len > end_addr) {
> +               puts("\nwget error: ");
> +               puts("trying to overwrite reserved memory...\n");
> +               return -1;
> +       }
> +#endif
> +       ptr = map_sysmem(store_addr, len);
>         memcpy(ptr, src, len);
>         unmap_sysmem(ptr);
>
> @@ -240,25 +284,37 @@ static void wget_connected(uchar *pkt, unsigned int tcp_seq_num,
>
>                         net_boot_file_size = 0;
>
> -                       if (len > hlen)
> -                               store_block(pkt + hlen, 0, len - hlen);
> +                       if (len > hlen) {
> +                               if (store_block(pkt + hlen, 0, len - hlen) != 0) {
> +                                       wget_loop_state = NETLOOP_FAIL;
> +                                       wget_fail("wget: store error\n", tcp_seq_num, tcp_ack_num, action);
> +                                       return;
> +                               }
> +                       }
>
>                         debug_cond(DEBUG_WGET,
>                                    "wget: Connected Pkt %p hlen %x\n",
>                                    pkt, hlen);
>
>                         for (i = 0; i < pkt_q_idx; i++) {
> +                               int err;
> +
>                                 ptr1 = map_sysmem(
>                                         (phys_addr_t)(pkt_q[i].pkt),
>                                         pkt_q[i].len);
> -                               store_block(ptr1,
> -                                           pkt_q[i].tcp_seq_num -
> -                                           initial_data_seq_num,
> -                                           pkt_q[i].len);
> +                               err = store_block(ptr1,
> +                                                 pkt_q[i].tcp_seq_num -
> +                                                 initial_data_seq_num,
> +                                                 pkt_q[i].len);
>                                 unmap_sysmem(ptr1);
>                                 debug_cond(DEBUG_WGET,
>                                            "wget: Connctd pkt Q %p len %x\n",
>                                            pkt_q[i].pkt, pkt_q[i].len);
> +                               if (err) {
> +                                       wget_loop_state = NETLOOP_FAIL;
> +                                       wget_fail("wget: store error\n", tcp_seq_num, tcp_ack_num, action);
> +                                       return;
> +                               }
>                         }
>                 }
>         }
> @@ -420,6 +476,12 @@ void wget_start(void)
>         debug_cond(DEBUG_WGET,
>                    "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
>
> +       if (wget_init_load_size()) {
> +               puts("\nwget error: ");
> +               puts("trying to overwrite reserved memory...\n");

I think it is OK to use printf() here.

> +               return;

Needs to return the error code

> +       }
> +
>         net_set_timeout_handler(wget_timeout, wget_timeout_handler);
>         tcp_set_tcp_handler(wget_handler);
>
> --
> 2.34.1
>

Regards,
Simon

Re: [PATCH v2 3/6] blk: blkmap: add ramdisk creation utility function

[Simon Glass]

On Fri, 1 Sept 2023 at 04:26, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> User needs to call several functions to create the ramdisk
> with blkmap.
> This adds the utility function to create blkmap device and
> mount the ramdisk.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  drivers/block/Makefile        |  1 +
>  drivers/block/blkmap.c        | 15 ----------
>  drivers/block/blkmap_helper.c | 53 +++++++++++++++++++++++++++++++++++
>  include/blkmap.h              | 29 +++++++++++++++++++
>  4 files changed, 83 insertions(+), 15 deletions(-)
>  create mode 100644 drivers/block/blkmap_helper.c
>

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v2 1/6] net: wget: prevent overwriting reserved memory

[Masahisa Kojima]

Hi Simon,

On Sat, 2 Sept 2023 at 09:09, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Masahisa,
>
> On Fri, 1 Sept 2023 at 04:26, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > This introduces the valid range check to store the received
> > blocks using lmb. The same logic is implemented in tftp.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >  net/wget.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 69 insertions(+), 7 deletions(-)
> >
> > diff --git a/net/wget.c b/net/wget.c
> > index 2dbfeb1a1d..8bf4db4d04 100644
> > --- a/net/wget.c
> > +++ b/net/wget.c
> > @@ -4,16 +4,20 @@
> >   * Copyright Duncan Hare <dh@synoia.com> 2017
> >   */
> >
> > +#include <asm/global_data.h>
> >  #include <command.h>
> >  #include <common.h>
> >  #include <display_options.h>
> >  #include <env.h>
> >  #include <image.h>
> > +#include <lmb.h>
> >  #include <mapmem.h>
> >  #include <net.h>
> >  #include <net/tcp.h>
> >  #include <net/wget.h>
> >
> > +DECLARE_GLOBAL_DATA_PTR;
> > +
> >  static const char bootfile1[] = "GET ";
> >  static const char bootfile3[] = " HTTP/1.0\r\n\r\n";
> >  static const char http_eom[] = "\r\n\r\n";
> > @@ -55,6 +59,32 @@ static unsigned int retry_tcp_ack_num;       /* TCP retry acknowledge number*/
> >  static unsigned int retry_tcp_seq_num; /* TCP retry sequence number */
> >  static int retry_len;                  /* TCP retry length */
> >
> > +#ifdef CONFIG_LMB
> > +static ulong wget_load_size;
> > +#endif
> > +
> > +/**
> > + * wget_init_max_size() - initialize maximum load size
> > + *
> > + * Return:     0 if success, -1 if fails
> > + */
> > +static int wget_init_load_size(void)
> > +{
> > +#ifdef CONFIG_LMB
>
> please can you use if IS_ENABLED() throughout?
OK.

>
> > +       struct lmb lmb;
> > +       phys_size_t max_size;
> > +
> > +       lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);
> > +
> > +       max_size = lmb_get_free_size(&lmb, image_load_addr);
> > +       if (!max_size)
> > +               return -1;
> > +
> > +       wget_load_size = max_size;
> > +#endif
> > +       return 0;
> > +}
> > +
> >  /**
> >   * store_block() - store block in memory
> >   * @src: source of data
> > @@ -63,10 +93,24 @@ static int retry_len;                       /* TCP retry length */
> >   */
> >  static inline int store_block(uchar *src, unsigned int offset, unsigned int len)
> >  {
> > +       ulong store_addr = image_load_addr + offset;
> >         ulong newsize = offset + len;
> >         uchar *ptr;
> >
> > -       ptr = map_sysmem(image_load_addr + offset, len);
> > +#ifdef CONFIG_LMB
> > +       ulong end_addr = image_load_addr + wget_load_size;
> > +
> > +       if (!end_addr)
> > +               end_addr = ULONG_MAX;
> > +
> > +       if (store_addr < image_load_addr ||
> > +               store_addr + len > end_addr) {
> > +               puts("\nwget error: ");
> > +               puts("trying to overwrite reserved memory...\n");
> > +               return -1;
> > +       }
> > +#endif
> > +       ptr = map_sysmem(store_addr, len);
> >         memcpy(ptr, src, len);
> >         unmap_sysmem(ptr);
> >
> > @@ -240,25 +284,37 @@ static void wget_connected(uchar *pkt, unsigned int tcp_seq_num,
> >
> >                         net_boot_file_size = 0;
> >
> > -                       if (len > hlen)
> > -                               store_block(pkt + hlen, 0, len - hlen);
> > +                       if (len > hlen) {
> > +                               if (store_block(pkt + hlen, 0, len - hlen) != 0) {
> > +                                       wget_loop_state = NETLOOP_FAIL;
> > +                                       wget_fail("wget: store error\n", tcp_seq_num, tcp_ack_num, action);
> > +                                       return;
> > +                               }
> > +                       }
> >
> >                         debug_cond(DEBUG_WGET,
> >                                    "wget: Connected Pkt %p hlen %x\n",
> >                                    pkt, hlen);
> >
> >                         for (i = 0; i < pkt_q_idx; i++) {
> > +                               int err;
> > +
> >                                 ptr1 = map_sysmem(
> >                                         (phys_addr_t)(pkt_q[i].pkt),
> >                                         pkt_q[i].len);
> > -                               store_block(ptr1,
> > -                                           pkt_q[i].tcp_seq_num -
> > -                                           initial_data_seq_num,
> > -                                           pkt_q[i].len);
> > +                               err = store_block(ptr1,
> > +                                                 pkt_q[i].tcp_seq_num -
> > +                                                 initial_data_seq_num,
> > +                                                 pkt_q[i].len);
> >                                 unmap_sysmem(ptr1);
> >                                 debug_cond(DEBUG_WGET,
> >                                            "wget: Connctd pkt Q %p len %x\n",
> >                                            pkt_q[i].pkt, pkt_q[i].len);
> > +                               if (err) {
> > +                                       wget_loop_state = NETLOOP_FAIL;
> > +                                       wget_fail("wget: store error\n", tcp_seq_num, tcp_ack_num, action);
> > +                                       return;
> > +                               }
> >                         }
> >                 }
> >         }
> > @@ -420,6 +476,12 @@ void wget_start(void)
> >         debug_cond(DEBUG_WGET,
> >                    "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
> >
> > +       if (wget_init_load_size()) {
> > +               puts("\nwget error: ");
> > +               puts("trying to overwrite reserved memory...\n");
>
> I think it is OK to use printf() here.
OK.

>
> > +               return;
>
> Needs to return the error code

I will modify it to notify the caller of the  error by calling
"net_set_state(NETLOOP_FAIL);"
instead of returning the error code.

Thanks,
Masahisa Kojima

>
> > +       }
> > +
> >         net_set_timeout_handler(wget_timeout, wget_timeout_handler);
> >         tcp_set_tcp_handler(wget_handler);
> >
> > --
> > 2.34.1
> >
>
> Regards,
> Simon

Re: [PATCH v2 2/6] net: wget: add wget with dns utility function

[Ilias Apalodimas]

Kojima-san

[...]

>  }
> +
> +#if (IS_ENABLED(CONFIG_CMD_DNS))
> +int wget_with_dns(ulong dst_addr, char *uri)
> +{
> +	int ret;
> +	char *s, *host_name, *file_name, *str_copy;
> +
> +	/*
> +	 * Download file using wget.
> +	 *
> +	 * 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.
> +	 */
> +	str_copy = strdup(uri);
> +	if (!str_copy)
> +		return -ENOMEM;
> +
> +	s = str_copy + strlen("http://");
> +	host_name = strsep(&s, "/");
> +	if (!s) {
> +		log_err("Error: invalied uri, no file path\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	file_name = s;
> +
> +	/* TODO: If the given uri has ip address for the http server, skip dns */
> +	net_dns_resolve = host_name;
> +	net_dns_env_var = "httpserverip";

This is not idea, but I understand this is how dns currently works.  We
can take another look on improving this when LWIP lands

> +	if (net_loop(DNS) < 0) {
> +		log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	s = env_get("httpserverip");
> +	if (!s) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	strlcpy(net_boot_file_name, s, 1024);

sizeof(net_boot_file_name) here please

> +	strlcat(net_boot_file_name, ":/", 1024); /* append '/' which is removed by strsep() */
> +	strlcat(net_boot_file_name, file_name, 1024);

Don't you have to limit the size of subsequent writes depending on what
previous calls wrote? IOW that can't always be 1024.

> +	image_load_addr = dst_addr;
> +	ret = net_loop(WGET);
> +
> +out:
> +	free(str_copy);
> +
> +	return ret;
> +}
> +#endif
> --
> 2.34.1
>

Thanks
/Ilias

Re: [PATCH v2 4/6] efi_loader: support boot from URI device path

[Ilias Apalodimas]

Kojima-san

On Fri, Sep 01, 2023 at 07:25:40PM +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.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  lib/efi_loader/efi_bootmgr.c | 197 +++++++++++++++++++++++++++++++++++
>  1 file changed, 197 insertions(+)
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a40762c74c..0e8d2ca9d1 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,193 @@ out:
>  	return ret;
>  }
>
> +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> +/**
> + * mount_image() - mount the image with blkmap
> + *
> + * @lo_label	u16 label string of load option
> + * @image_addr:	image address
> + * @image_size	image size
> + * Return:	pointer to the UCLASS_BLK udevice, NULL if failed
> + */
> +static struct udevice *mount_image(u16 *lo_label, ulong image_addr, int image_size)
> +{
> +	int err;
> +	struct blkmap *bm;
> +	struct udevice *bm_dev;
> +	char *label = NULL, *p;
> +
> +	label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> +	if (!label)
> +		return NULL;
> +
> +	p = label;
> +	utf16_utf8_strcpy(&p, lo_label);
> +	err = blkmap_create_ramdisk(label, image_addr, image_size, &bm_dev);
> +	if (err) {
> +		efi_free_pool(label);
> +		return NULL;
> +	}
> +	bm = dev_get_plat(bm_dev);
> +
> +	efi_free_pool(label);
> +
> +	return bm->blk;
> +}
> +
> +/**
> + * try_load_default_file() - try to load the default file
> + *
> + * Search the device having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> + *
> + * @dev			pointer to the UCLASS_BLK or UCLASS_PARTITION udevice
> + * @image_handle:	pointer to handle for newly installed image
> + * Return:		status code
> + */
> +static efi_status_t try_load_default_file(struct udevice *dev,
> +					  efi_handle_t *image_handle)
> +{
> +	efi_status_t ret;
> +	efi_handle_t bm_handle;
> +	struct efi_handler *handler;
> +	struct efi_device_path *file_path;
> +	struct efi_device_path *device_path;
> +
> +	if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&bm_handle)) {
> +		log_warning("DM_TAG_EFI not found\n");
> +		return EFI_INVALID_PARAMETER;
> +	}
> +
> +	ret = efi_search_protocol(bm_handle,
> +				  &efi_simple_file_system_protocol_guid, &handler);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	ret = efi_search_protocol(bm_handle, &efi_guid_device_path, &handler);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	ret = efi_protocol_open(handler, (void **)&device_path, efi_root, NULL,
> +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	file_path = expand_media_path(device_path);
> +	ret = EFI_CALL(efi_load_image(true, efi_root, file_path, NULL, 0,
> +				      image_handle));
> +
> +	efi_free_pool(file_path);
> +
> +	return ret;
> +}

We need to decide what we want here.  There were recently some patches from
Raymond [0] which piggybacked on your earlier eficonfig work.  I think the
last patch of this series hasn't been merged due to a test failing, but we
should fix it.
That patch has a different approach.  Everytime a disk appears it will add
a boot option if the default filepath is found and that's how we fixed the
behaviour of efibootmgr to adhere to the EFI spec.  This patch is doing the
opposite, trying to detect the BOOTAA64.EFI etc on the fly.  I think I
prefer the approach you have here, but we should end up with one solution
to solve both.

> +
> +/**
> + * load_default_file_from_blk_dev() - load the default file
> + *
> + * @blk		pointer to the UCLASS_BLK udevice
> + * @handle:	pointer to handle for newly installed image
> + * Return:	status code
> + */
> +static efi_status_t load_default_file_from_blk_dev(struct udevice *blk,
> +						   efi_handle_t *handle)
> +{
> +	efi_status_t ret;
> +	struct udevice *partition;
> +
> +	/* image that has no partition table but a file system */
> +	ret = try_load_default_file(blk, handle);
> +	if (ret == EFI_SUCCESS)
> +		return ret;
> +
> +	/* try the partitions */
> +	device_foreach_child(partition, blk) {
> +		enum uclass_id id;
> +
> +		id = device_get_uclass_id(partition);
> +		if (id != UCLASS_PARTITION)
> +			continue;
> +
> +		ret = try_load_default_file(partition, handle);
> +		if (ret == EFI_SUCCESS)
> +			return ret;
> +	}
> +
> +	return EFI_NOT_FOUND;
> +}
> +
> +/**
> + * 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)
> +{
> +	char *s;
> +	int uri_len;
> +	int image_size;
> +	efi_status_t ret;
> +	ulong image_addr;
> +
> +	s = env_get("loadaddr");
> +	if (!s) {
> +		log_err("Error: loadaddr is not set\n");
> +		return EFI_INVALID_PARAMETER;
> +	}
> +	image_addr = hextoul(s, NULL);
> +	image_size = wget_with_dns(image_addr, uridp->uri);
> +	if (image_size < 0)
> +		return EFI_INVALID_PARAMETER;
> +
> +	/*
> +	 * If the file extension is ".iso" or ".img", mount it and try to load
> +	 * the default file.

Don't we have a better way to validate isos and efi apps instead of
the extension?  The efi is checked against a valid PE/COFF image so I guess
we'll really on the mount to fail for isos?

> +	 * If the file is ".efi" and PE-COFF image, load the downloaded file.
> +	 */
> +	uri_len = strlen(uridp->uri); /* todo: directly use uridp->uri */
> +	if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
> +	    !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
> +		struct udevice *blk;
> +
> +		blk = mount_image(lo_label, image_addr, image_size);
> +		if (!blk)
> +			return EFI_INVALID_PARAMETER;
> +
> +		ret = load_default_file_from_blk_dev(blk, handle);
> +	} else if (!strncmp(&uridp->uri[uri_len - 4], ".efi", 4)) {
> +		efi_handle_t mem_handle = NULL;
> +		struct efi_device_path *file_path = NULL;
> +
> +		ret = efi_check_pe((void *)image_addr, image_size, NULL);
> +		if (ret != EFI_SUCCESS) {
> +			log_err("Error: downloaded image is not a PE-COFF image\n");
> +			return EFI_INVALID_PARAMETER;
> +		}
> +
> +		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +					    (uintptr_t)image_addr, image_size);
> +		ret = efi_install_multiple_protocol_interfaces(
> +			&mem_handle, &efi_guid_device_path, file_path, NULL);
> +		if (ret != EFI_SUCCESS)
> +			return EFI_INVALID_PARAMETER;
> +
> +		ret = EFI_CALL(efi_load_image(false, efi_root, file_path,
> +					      (void *)image_addr, image_size,

[0] https://lore.kernel.org/u-boot/20230619212303.128288-1-raymond.mao@linaro.org/

Thanks
/Ilias

Re: [PATCH v2 6/6] doc: uefi: add HTTP Boot support

[Ilias Apalodimas]

On Fri, Sep 01, 2023 at 07:25:42PM +0900, Masahisa Kojima wrote:
> This adds the description about HTTP Boot.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  doc/develop/uefi/uefi.rst | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
> index a7a41f2fac..fd87bb1edb 100644
> --- a/doc/develop/uefi/uefi.rst
> +++ b/doc/develop/uefi/uefi.rst
> @@ -594,6 +594,27 @@ UEFI variables. Booting according to these variables is possible via::
>  As of U-Boot v2020.10 UEFI variables cannot be set at runtime. The U-Boot
>  command 'efidebug' can be used to set the variables.
>
> +UEFI HTTP Boot
> +~~~~~~~~~~~~~~
> +
> +HTTP Boot provides the capability for system deployment and configuration
> +over the network. HTTP Boot can be activated by specifying::
> +
> +    CONFIG_CMD_DNS
> +    CONFIG_CMD_WGET
> +    CONFIG_BLKMAP
> +
> +Set up the load option specifying the target URI::
> +
> +    efidebug boot add -u 1 netinst http://foo/bar
> +
> +When this load option is selected as boot selection, resolve the
> +host ip address by dhs, then download the file with wget.

 dhs -> dns
 what happens if it's an IP address?

> +If the downloaded file extension is .iso or .img file, efibootmgr tries to
> +mount the image and boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> +If the downloaded file extension is .efi and file is PE-COFF image,
> +load the downloaded file and start it.
> +
>  Executing the built in hello world application
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> --
> 2.34.1
>

Thanks
/Ilias

Re: [PATCH v2 4/6] efi_loader: support boot from URI device path

[Heinrich Schuchardt]

On 01.09.23 12:25, 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 | 197 +++++++++++++++++++++++++++++++++++
>   1 file changed, 197 insertions(+)
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a40762c74c..0e8d2ca9d1 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,193 @@ out:
>   	return ret;
>   }
>
> +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> +/**
> + * mount_image() - mount the image with blkmap
> + *
> + * @lo_label	u16 label string of load option
> + * @image_addr:	image address
> + * @image_size	image size
> + * Return:	pointer to the UCLASS_BLK udevice, NULL if failed
> + */
> +static struct udevice *mount_image(u16 *lo_label, ulong image_addr, int image_size)
> +{
> +	int err;
> +	struct blkmap *bm;
> +	struct udevice *bm_dev;
> +	char *label = NULL, *p;
> +
> +	label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> +	if (!label)
> +		return NULL;
> +
> +	p = label;
> +	utf16_utf8_strcpy(&p, lo_label);
> +	err = blkmap_create_ramdisk(label, image_addr, image_size, &bm_dev);
> +	if (err) {
> +		efi_free_pool(label);
> +		return NULL;
> +	}
> +	bm = dev_get_plat(bm_dev);
> +
> +	efi_free_pool(label);
> +
> +	return bm->blk;
> +}
> +
> +/**
> + * try_load_default_file() - try to load the default file
> + *
> + * Search the device having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> + *
> + * @dev			pointer to the UCLASS_BLK or UCLASS_PARTITION udevice
> + * @image_handle:	pointer to handle for newly installed image
> + * Return:		status code
> + */
> +static efi_status_t try_load_default_file(struct udevice *dev,
> +					  efi_handle_t *image_handle)
> +{
> +	efi_status_t ret;
> +	efi_handle_t bm_handle;
> +	struct efi_handler *handler;
> +	struct efi_device_path *file_path;
> +	struct efi_device_path *device_path;
> +
> +	if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&bm_handle)) {
> +		log_warning("DM_TAG_EFI not found\n");
> +		return EFI_INVALID_PARAMETER;
> +	}
> +
> +	ret = efi_search_protocol(bm_handle,
> +				  &efi_simple_file_system_protocol_guid, &handler);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	ret = efi_search_protocol(bm_handle, &efi_guid_device_path, &handler);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	ret = efi_protocol_open(handler, (void **)&device_path, efi_root, NULL,
> +				EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	file_path = expand_media_path(device_path);
> +	ret = EFI_CALL(efi_load_image(true, efi_root, file_path, NULL, 0,
> +				      image_handle));
> +
> +	efi_free_pool(file_path);
> +
> +	return ret;
> +}
> +
> +/**
> + * load_default_file_from_blk_dev() - load the default file
> + *
> + * @blk		pointer to the UCLASS_BLK udevice
> + * @handle:	pointer to handle for newly installed image
> + * Return:	status code
> + */
> +static efi_status_t load_default_file_from_blk_dev(struct udevice *blk,
> +						   efi_handle_t *handle)
> +{
> +	efi_status_t ret;
> +	struct udevice *partition;
> +
> +	/* image that has no partition table but a file system */
> +	ret = try_load_default_file(blk, handle);
> +	if (ret == EFI_SUCCESS)
> +		return ret;
> +
> +	/* try the partitions */
> +	device_foreach_child(partition, blk) {
> +		enum uclass_id id;
> +
> +		id = device_get_uclass_id(partition);
> +		if (id != UCLASS_PARTITION)
> +			continue;
> +
> +		ret = try_load_default_file(partition, handle);
> +		if (ret == EFI_SUCCESS)
> +			return ret;
> +	}
> +
> +	return EFI_NOT_FOUND;
> +}
> +
> +/**
> + * 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)
> +{
> +	char *s;
> +	int uri_len;
> +	int image_size;
> +	efi_status_t ret;
> +	ulong image_addr;
> +
> +	s = env_get("loadaddr");
> +	if (!s) {
> +		log_err("Error: loadaddr is not set\n");
> +		return EFI_INVALID_PARAMETER;
> +	}
> +	image_addr = hextoul(s, NULL);
> +	image_size = wget_with_dns(image_addr, uridp->uri);
> +	if (image_size < 0)
> +		return EFI_INVALID_PARAMETER;
> +
> +	/*
> +	 * If the file extension is ".iso" or ".img", mount it and try to load
> +	 * the default file.
> +	 * If the file is ".efi" and PE-COFF image, load the downloaded file.
> +	 */

There is no requirement that EFI binaries have an .efi extension.
Does EDK II require it?

Best regards

Heinrich


> +	uri_len = strlen(uridp->uri); /* todo: directly use uridp->uri */
> +	if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
> +	    !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
> +		struct udevice *blk;
> +
> +		blk = mount_image(lo_label, image_addr, image_size);
> +		if (!blk)
> +			return EFI_INVALID_PARAMETER;
> +
> +		ret = load_default_file_from_blk_dev(blk, handle);
> +	} else if (!strncmp(&uridp->uri[uri_len - 4], ".efi", 4)) {
> +		efi_handle_t mem_handle = NULL;
> +		struct efi_device_path *file_path = NULL;
> +
> +		ret = efi_check_pe((void *)image_addr, image_size, NULL);
> +		if (ret != EFI_SUCCESS) {
> +			log_err("Error: downloaded image is not a PE-COFF image\n");
> +			return EFI_INVALID_PARAMETER;
> +		}
> +
> +		file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> +					    (uintptr_t)image_addr, image_size);
> +		ret = efi_install_multiple_protocol_interfaces(
> +			&mem_handle, &efi_guid_device_path, file_path, NULL);
> +		if (ret != EFI_SUCCESS)
> +			return EFI_INVALID_PARAMETER;
> +
> +		ret = EFI_CALL(efi_load_image(false, efi_root, file_path,
> +					      (void *)image_addr, image_size,
> +					      handle));
> +	} else {
> +		log_err("Error: file type is not supported\n");
> +		return EFI_INVALID_PARAMETER;
> +	}
> +
> +	return ret;
> +}
> +#endif
> +
>   /**
>    * try_load_entry() - try to load image for boot option
>    *
> @@ -211,6 +402,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 v2 2/6] net: wget: add wget with dns utility function

[Masahisa Kojima]

Hi Ilias,

On Thu, 14 Sept 2023 at 21:52, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Kojima-san
>
> [...]
>
> >  }
> > +
> > +#if (IS_ENABLED(CONFIG_CMD_DNS))
> > +int wget_with_dns(ulong dst_addr, char *uri)
> > +{
> > +     int ret;
> > +     char *s, *host_name, *file_name, *str_copy;
> > +
> > +     /*
> > +      * Download file using wget.
> > +      *
> > +      * 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.
> > +      */
> > +     str_copy = strdup(uri);
> > +     if (!str_copy)
> > +             return -ENOMEM;
> > +
> > +     s = str_copy + strlen("http://");
> > +     host_name = strsep(&s, "/");
> > +     if (!s) {
> > +             log_err("Error: invalied uri, no file path\n");
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +     file_name = s;
> > +
> > +     /* TODO: If the given uri has ip address for the http server, skip dns */
> > +     net_dns_resolve = host_name;
> > +     net_dns_env_var = "httpserverip";
>
> This is not idea, but I understand this is how dns currently works.  We
> can take another look on improving this when LWIP lands

Yes.
Note that the prototype of this function is the same as the  Maxim's LWIP port
"int ulwip_wget(ulong addr, char *url)", so we can easily replace it
when the LWIP lands.

>
> > +     if (net_loop(DNS) < 0) {
> > +             log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +     s = env_get("httpserverip");
> > +     if (!s) {
> > +             ret = -EINVAL;
> > +             goto out;
> > +     }
> > +
> > +     strlcpy(net_boot_file_name, s, 1024);
>
> sizeof(net_boot_file_name) here please
OK.

>
> > +     strlcat(net_boot_file_name, ":/", 1024); /* append '/' which is removed by strsep() */
> > +     strlcat(net_boot_file_name, file_name, 1024);
>
> Don't you have to limit the size of subsequent writes depending on what
> previous calls wrote? IOW that can't always be 1024.

strlcat() appends the string with the limit of "buf_size - strlen(dst)
- 1", so current code should be OK.
1024 should be sizeof(net_boot_file_name) same as above.

Thanks,
Masahisa Kojima

>
> > +     image_load_addr = dst_addr;
> > +     ret = net_loop(WGET);
> > +
> > +out:
> > +     free(str_copy);
> > +
> > +     return ret;
> > +}
> > +#endif
> > --
> > 2.34.1
> >
>
> Thanks
> /Ilias

Re: [PATCH v2 6/6] doc: uefi: add HTTP Boot support

[Masahisa Kojima]

On Thu, 14 Sept 2023 at 22:57, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, Sep 01, 2023 at 07:25:42PM +0900, Masahisa Kojima wrote:
> > This adds the description about HTTP Boot.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >  doc/develop/uefi/uefi.rst | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
> > index a7a41f2fac..fd87bb1edb 100644
> > --- a/doc/develop/uefi/uefi.rst
> > +++ b/doc/develop/uefi/uefi.rst
> > @@ -594,6 +594,27 @@ UEFI variables. Booting according to these variables is possible via::
> >  As of U-Boot v2020.10 UEFI variables cannot be set at runtime. The U-Boot
> >  command 'efidebug' can be used to set the variables.
> >
> > +UEFI HTTP Boot
> > +~~~~~~~~~~~~~~
> > +
> > +HTTP Boot provides the capability for system deployment and configuration
> > +over the network. HTTP Boot can be activated by specifying::
> > +
> > +    CONFIG_CMD_DNS
> > +    CONFIG_CMD_WGET
> > +    CONFIG_BLKMAP
> > +
> > +Set up the load option specifying the target URI::
> > +
> > +    efidebug boot add -u 1 netinst http://foo/bar
> > +
> > +When this load option is selected as boot selection, resolve the
> > +host ip address by dhs, then download the file with wget.
>
>  dhs -> dns
OK.

>  what happens if it's an IP address?
Current implementation tries to resolve the IP address as a host name,
so the DNS process will end up with "host not found".
We need to preset the "httpserverip" env variable to proceed the wget.

For this series, without a lwip port, this limitation should be noted here.

I could take the ip address format check code from iwip or other projects,
but I did not do that since there is a workaround(set "httpserverip"
env variable).
In addition, when the lwip port lands, this case is properly handled.
If the IP address is used as host, the DNS process is skipped.

Thanks,
Masahisa Kojima

>
> > +If the downloaded file extension is .iso or .img file, efibootmgr tries to
> > +mount the image and boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > +If the downloaded file extension is .efi and file is PE-COFF image,
> > +load the downloaded file and start it.
> > +
> >  Executing the built in hello world application
> >  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > --
> > 2.34.1
> >
>
> Thanks
> /Ilias

Re: [PATCH v2 4/6] efi_loader: support boot from URI device path

[Masahisa Kojima]

Hi Ilias,

On Thu, 14 Sept 2023 at 22:56, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Kojima-san
>
> On Fri, Sep 01, 2023 at 07:25:40PM +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.
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >  lib/efi_loader/efi_bootmgr.c | 197 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 197 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a40762c74c..0e8d2ca9d1 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,193 @@ out:
> >       return ret;
> >  }
> >
> > +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> > +/**
> > + * mount_image() - mount the image with blkmap
> > + *
> > + * @lo_label u16 label string of load option
> > + * @image_addr:      image address
> > + * @image_size       image size
> > + * Return:   pointer to the UCLASS_BLK udevice, NULL if failed
> > + */
> > +static struct udevice *mount_image(u16 *lo_label, ulong image_addr, int image_size)
> > +{
> > +     int err;
> > +     struct blkmap *bm;
> > +     struct udevice *bm_dev;
> > +     char *label = NULL, *p;
> > +
> > +     label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> > +     if (!label)
> > +             return NULL;
> > +
> > +     p = label;
> > +     utf16_utf8_strcpy(&p, lo_label);
> > +     err = blkmap_create_ramdisk(label, image_addr, image_size, &bm_dev);
> > +     if (err) {
> > +             efi_free_pool(label);
> > +             return NULL;
> > +     }
> > +     bm = dev_get_plat(bm_dev);
> > +
> > +     efi_free_pool(label);
> > +
> > +     return bm->blk;
> > +}
> > +
> > +/**
> > + * try_load_default_file() - try to load the default file
> > + *
> > + * Search the device having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> > + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > + *
> > + * @dev                      pointer to the UCLASS_BLK or UCLASS_PARTITION udevice
> > + * @image_handle:    pointer to handle for newly installed image
> > + * Return:           status code
> > + */
> > +static efi_status_t try_load_default_file(struct udevice *dev,
> > +                                       efi_handle_t *image_handle)
> > +{
> > +     efi_status_t ret;
> > +     efi_handle_t bm_handle;
> > +     struct efi_handler *handler;
> > +     struct efi_device_path *file_path;
> > +     struct efi_device_path *device_path;
> > +
> > +     if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&bm_handle)) {
> > +             log_warning("DM_TAG_EFI not found\n");
> > +             return EFI_INVALID_PARAMETER;
> > +     }
> > +
> > +     ret = efi_search_protocol(bm_handle,
> > +                               &efi_simple_file_system_protocol_guid, &handler);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     ret = efi_search_protocol(bm_handle, &efi_guid_device_path, &handler);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     ret = efi_protocol_open(handler, (void **)&device_path, efi_root, NULL,
> > +                             EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     file_path = expand_media_path(device_path);
> > +     ret = EFI_CALL(efi_load_image(true, efi_root, file_path, NULL, 0,
> > +                                   image_handle));
> > +
> > +     efi_free_pool(file_path);
> > +
> > +     return ret;
> > +}
>
> We need to decide what we want here.  There were recently some patches from
> Raymond [0] which piggybacked on your earlier eficonfig work.  I think the
> last patch of this series hasn't been merged due to a test failing, but we
> should fix it.
> That patch has a different approach.  Everytime a disk appears it will add
> a boot option if the default filepath is found and that's how we fixed the

efi_bootmgr_update_media_device_boot_option() automatically adds the
boot option if the device handle installed EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
but it does not check whether the default file path is found or not.
When the user selects the automatically created boot option,
expand_media_path()
is called and efibootmgr tries to boot with the default file.

> behaviour of efibootmgr to adhere to the EFI spec.  This patch is doing the
> opposite, trying to detect the BOOTAA64.EFI etc on the fly.  I think I
> prefer the approach you have here, but we should end up with one solution
> to solve both.

This HTTP Boot case is slightly different from the case where the user selects
the automatically added boot option.
In this case, user selects the URI device path boot option. The
efibootmgr downloads the
file, mount the image, and try to boot with the default file name on the fly.
When the patch[0] is merged, it is possible to boot the downloaded iso
image from the
automatically added "blkmap" boot option, but I think efibootmgr needs
to boot with
the URI device path user selected that this series does, not boot
from the "blkmap" boot option.

>
> > +
> > +/**
> > + * load_default_file_from_blk_dev() - load the default file
> > + *
> > + * @blk              pointer to the UCLASS_BLK udevice
> > + * @handle:  pointer to handle for newly installed image
> > + * Return:   status code
> > + */
> > +static efi_status_t load_default_file_from_blk_dev(struct udevice *blk,
> > +                                                efi_handle_t *handle)
> > +{
> > +     efi_status_t ret;
> > +     struct udevice *partition;
> > +
> > +     /* image that has no partition table but a file system */
> > +     ret = try_load_default_file(blk, handle);
> > +     if (ret == EFI_SUCCESS)
> > +             return ret;
> > +
> > +     /* try the partitions */
> > +     device_foreach_child(partition, blk) {
> > +             enum uclass_id id;
> > +
> > +             id = device_get_uclass_id(partition);
> > +             if (id != UCLASS_PARTITION)
> > +                     continue;
> > +
> > +             ret = try_load_default_file(partition, handle);
> > +             if (ret == EFI_SUCCESS)
> > +                     return ret;
> > +     }
> > +
> > +     return EFI_NOT_FOUND;
> > +}
> > +
> > +/**
> > + * 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)
> > +{
> > +     char *s;
> > +     int uri_len;
> > +     int image_size;
> > +     efi_status_t ret;
> > +     ulong image_addr;
> > +
> > +     s = env_get("loadaddr");
> > +     if (!s) {
> > +             log_err("Error: loadaddr is not set\n");
> > +             return EFI_INVALID_PARAMETER;
> > +     }
> > +     image_addr = hextoul(s, NULL);
> > +     image_size = wget_with_dns(image_addr, uridp->uri);
> > +     if (image_size < 0)
> > +             return EFI_INVALID_PARAMETER;
> > +
> > +     /*
> > +      * If the file extension is ".iso" or ".img", mount it and try to load
> > +      * the default file.
>
> Don't we have a better way to validate isos and efi apps instead of
> the extension?  The efi is checked against a valid PE/COFF image so I guess
> we'll really on the mount to fail for isos?

EDK2 reference implementation checks the file type with the following priority.
 1) "Content-Type" header in HTTP response message
 2) Filename Extensions
Documentation is here[1].

Since "Content-Type" is not available in the current U-Boot wget, file extension
is used to identify ISO images.

[1] https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot#ram-disk-boot-from-http

Thanks,
Masahisa Kojima


>
> > +      * If the file is ".efi" and PE-COFF image, load the downloaded file.
> > +      */
> > +     uri_len = strlen(uridp->uri); /* todo: directly use uridp->uri */
> > +     if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
> > +         !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
> > +             struct udevice *blk;
> > +
> > +             blk = mount_image(lo_label, image_addr, image_size);
> > +             if (!blk)
> > +                     return EFI_INVALID_PARAMETER;
> > +
> > +             ret = load_default_file_from_blk_dev(blk, handle);
> > +     } else if (!strncmp(&uridp->uri[uri_len - 4], ".efi", 4)) {
> > +             efi_handle_t mem_handle = NULL;
> > +             struct efi_device_path *file_path = NULL;
> > +
> > +             ret = efi_check_pe((void *)image_addr, image_size, NULL);
> > +             if (ret != EFI_SUCCESS) {
> > +                     log_err("Error: downloaded image is not a PE-COFF image\n");
> > +                     return EFI_INVALID_PARAMETER;
> > +             }
> > +
> > +             file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> > +                                         (uintptr_t)image_addr, image_size);
> > +             ret = efi_install_multiple_protocol_interfaces(
> > +                     &mem_handle, &efi_guid_device_path, file_path, NULL);
> > +             if (ret != EFI_SUCCESS)
> > +                     return EFI_INVALID_PARAMETER;
> > +
> > +             ret = EFI_CALL(efi_load_image(false, efi_root, file_path,
> > +                                           (void *)image_addr, image_size,
>
> [0] https://lore.kernel.org/u-boot/20230619212303.128288-1-raymond.mao@linaro.org/
>
> Thanks
> /Ilias

Re: [PATCH v2 4/6] efi_loader: support boot from URI device path

[Masahisa Kojima]

Hi Heinrich,

On Thu, 14 Sept 2023 at 23:55, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 01.09.23 12:25, 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 | 197 +++++++++++++++++++++++++++++++++++
> >   1 file changed, 197 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a40762c74c..0e8d2ca9d1 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,193 @@ out:
> >       return ret;
> >   }
> >
> > +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> > +/**
> > + * mount_image() - mount the image with blkmap
> > + *
> > + * @lo_label u16 label string of load option
> > + * @image_addr:      image address
> > + * @image_size       image size
> > + * Return:   pointer to the UCLASS_BLK udevice, NULL if failed
> > + */
> > +static struct udevice *mount_image(u16 *lo_label, ulong image_addr, int image_size)
> > +{
> > +     int err;
> > +     struct blkmap *bm;
> > +     struct udevice *bm_dev;
> > +     char *label = NULL, *p;
> > +
> > +     label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> > +     if (!label)
> > +             return NULL;
> > +
> > +     p = label;
> > +     utf16_utf8_strcpy(&p, lo_label);
> > +     err = blkmap_create_ramdisk(label, image_addr, image_size, &bm_dev);
> > +     if (err) {
> > +             efi_free_pool(label);
> > +             return NULL;
> > +     }
> > +     bm = dev_get_plat(bm_dev);
> > +
> > +     efi_free_pool(label);
> > +
> > +     return bm->blk;
> > +}
> > +
> > +/**
> > + * try_load_default_file() - try to load the default file
> > + *
> > + * Search the device having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> > + * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > + *
> > + * @dev                      pointer to the UCLASS_BLK or UCLASS_PARTITION udevice
> > + * @image_handle:    pointer to handle for newly installed image
> > + * Return:           status code
> > + */
> > +static efi_status_t try_load_default_file(struct udevice *dev,
> > +                                       efi_handle_t *image_handle)
> > +{
> > +     efi_status_t ret;
> > +     efi_handle_t bm_handle;
> > +     struct efi_handler *handler;
> > +     struct efi_device_path *file_path;
> > +     struct efi_device_path *device_path;
> > +
> > +     if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&bm_handle)) {
> > +             log_warning("DM_TAG_EFI not found\n");
> > +             return EFI_INVALID_PARAMETER;
> > +     }
> > +
> > +     ret = efi_search_protocol(bm_handle,
> > +                               &efi_simple_file_system_protocol_guid, &handler);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     ret = efi_search_protocol(bm_handle, &efi_guid_device_path, &handler);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     ret = efi_protocol_open(handler, (void **)&device_path, efi_root, NULL,
> > +                             EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     file_path = expand_media_path(device_path);
> > +     ret = EFI_CALL(efi_load_image(true, efi_root, file_path, NULL, 0,
> > +                                   image_handle));
> > +
> > +     efi_free_pool(file_path);
> > +
> > +     return ret;
> > +}
> > +
> > +/**
> > + * load_default_file_from_blk_dev() - load the default file
> > + *
> > + * @blk              pointer to the UCLASS_BLK udevice
> > + * @handle:  pointer to handle for newly installed image
> > + * Return:   status code
> > + */
> > +static efi_status_t load_default_file_from_blk_dev(struct udevice *blk,
> > +                                                efi_handle_t *handle)
> > +{
> > +     efi_status_t ret;
> > +     struct udevice *partition;
> > +
> > +     /* image that has no partition table but a file system */
> > +     ret = try_load_default_file(blk, handle);
> > +     if (ret == EFI_SUCCESS)
> > +             return ret;
> > +
> > +     /* try the partitions */
> > +     device_foreach_child(partition, blk) {
> > +             enum uclass_id id;
> > +
> > +             id = device_get_uclass_id(partition);
> > +             if (id != UCLASS_PARTITION)
> > +                     continue;
> > +
> > +             ret = try_load_default_file(partition, handle);
> > +             if (ret == EFI_SUCCESS)
> > +                     return ret;
> > +     }
> > +
> > +     return EFI_NOT_FOUND;
> > +}
> > +
> > +/**
> > + * 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)
> > +{
> > +     char *s;
> > +     int uri_len;
> > +     int image_size;
> > +     efi_status_t ret;
> > +     ulong image_addr;
> > +
> > +     s = env_get("loadaddr");
> > +     if (!s) {
> > +             log_err("Error: loadaddr is not set\n");
> > +             return EFI_INVALID_PARAMETER;
> > +     }
> > +     image_addr = hextoul(s, NULL);
> > +     image_size = wget_with_dns(image_addr, uridp->uri);
> > +     if (image_size < 0)
> > +             return EFI_INVALID_PARAMETER;
> > +
> > +     /*
> > +      * If the file extension is ".iso" or ".img", mount it and try to load
> > +      * the default file.
> > +      * If the file is ".efi" and PE-COFF image, load the downloaded file.
> > +      */
>
> There is no requirement that EFI binaries have an .efi extension.
> Does EDK II require it?

No, sorry.
".efi" extension check should be removed.

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
>
> > +     uri_len = strlen(uridp->uri); /* todo: directly use uridp->uri */
> > +     if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
> > +         !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
> > +             struct udevice *blk;
> > +
> > +             blk = mount_image(lo_label, image_addr, image_size);
> > +             if (!blk)
> > +                     return EFI_INVALID_PARAMETER;
> > +
> > +             ret = load_default_file_from_blk_dev(blk, handle);
> > +     } else if (!strncmp(&uridp->uri[uri_len - 4], ".efi", 4)) {
> > +             efi_handle_t mem_handle = NULL;
> > +             struct efi_device_path *file_path = NULL;
> > +
> > +             ret = efi_check_pe((void *)image_addr, image_size, NULL);
> > +             if (ret != EFI_SUCCESS) {
> > +                     log_err("Error: downloaded image is not a PE-COFF image\n");
> > +                     return EFI_INVALID_PARAMETER;
> > +             }
> > +
> > +             file_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE,
> > +                                         (uintptr_t)image_addr, image_size);
> > +             ret = efi_install_multiple_protocol_interfaces(
> > +                     &mem_handle, &efi_guid_device_path, file_path, NULL);
> > +             if (ret != EFI_SUCCESS)
> > +                     return EFI_INVALID_PARAMETER;
> > +
> > +             ret = EFI_CALL(efi_load_image(false, efi_root, file_path,
> > +                                           (void *)image_addr, image_size,
> > +                                           handle));
> > +     } else {
> > +             log_err("Error: file type is not supported\n");
> > +             return EFI_INVALID_PARAMETER;
> > +     }
> > +
> > +     return ret;
> > +}
> > +#endif
> > +
> >   /**
> >    * try_load_entry() - try to load image for boot option
> >    *
> > @@ -211,6 +402,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 v2 6/6] doc: uefi: add HTTP Boot support

[Ilias Apalodimas]

On Fri, 15 Sept 2023 at 02:52, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> On Thu, 14 Sept 2023 at 22:57, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Fri, Sep 01, 2023 at 07:25:42PM +0900, Masahisa Kojima wrote:
> > > This adds the description about HTTP Boot.
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > >  doc/develop/uefi/uefi.rst | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > >
> > > diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
> > > index a7a41f2fac..fd87bb1edb 100644
> > > --- a/doc/develop/uefi/uefi.rst
> > > +++ b/doc/develop/uefi/uefi.rst
> > > @@ -594,6 +594,27 @@ UEFI variables. Booting according to these variables is possible via::
> > >  As of U-Boot v2020.10 UEFI variables cannot be set at runtime. The U-Boot
> > >  command 'efidebug' can be used to set the variables.
> > >
> > > +UEFI HTTP Boot
> > > +~~~~~~~~~~~~~~
> > > +
> > > +HTTP Boot provides the capability for system deployment and configuration
> > > +over the network. HTTP Boot can be activated by specifying::
> > > +
> > > +    CONFIG_CMD_DNS
> > > +    CONFIG_CMD_WGET
> > > +    CONFIG_BLKMAP
> > > +
> > > +Set up the load option specifying the target URI::
> > > +
> > > +    efidebug boot add -u 1 netinst http://foo/bar
> > > +
> > > +When this load option is selected as boot selection, resolve the
> > > +host ip address by dhs, then download the file with wget.
> >
> >  dhs -> dns
> OK.
>
> >  what happens if it's an IP address?
> Current implementation tries to resolve the IP address as a host name,
> so the DNS process will end up with "host not found".
> We need to preset the "httpserverip" env variable to proceed the wget.
>
> For this series, without a lwip port, this limitation should be noted here.
>
> I could take the ip address format check code from iwip or other projects,
> but I did not do that since there is a workaround(set "httpserverip"
> env variable).
> In addition, when the lwip port lands, this case is properly handled.
> If the IP address is used as host, the DNS process is skipped.

Ok, then update this with information on how to use an IP address
instead of a hostname and once LWIP lands we can update it

Thanks
/Ilias
>
> Thanks,
> Masahisa Kojima
>
> >
> > > +If the downloaded file extension is .iso or .img file, efibootmgr tries to
> > > +mount the image and boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > > +If the downloaded file extension is .efi and file is PE-COFF image,
> > > +load the downloaded file and start it.
> > > +
> > >  Executing the built in hello world application
> > >  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > >
> > > --
> > > 2.34.1
> > >
> >
> > Thanks
> > /Ilias

Re: [PATCH v2 4/6] efi_loader: support boot from URI device path

[Ilias Apalodimas]

Hi Kojima-san, 

> > > +                                       efi_handle_t *image_handle)
> > > +{
> > > +     efi_status_t ret;
> > > +     efi_handle_t bm_handle;
> > > +     struct efi_handler *handler;
> > > +     struct efi_device_path *file_path;
> > > +     struct efi_device_path *device_path;
> > > +
> > > +     if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&bm_handle)) {
> > > +             log_warning("DM_TAG_EFI not found\n");
> > > +             return EFI_INVALID_PARAMETER;
> > > +     }
> > > +
> > > +     ret = efi_search_protocol(bm_handle,
> > > +                               &efi_simple_file_system_protocol_guid, &handler);
> > > +     if (ret != EFI_SUCCESS)
> > > +             return ret;
> > > +
> > > +     ret = efi_search_protocol(bm_handle, &efi_guid_device_path, &handler);
> > > +     if (ret != EFI_SUCCESS)
> > > +             return ret;
> > > +
> > > +     ret = efi_protocol_open(handler, (void **)&device_path, efi_root, NULL,
> > > +                             EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > > +     if (ret != EFI_SUCCESS)
> > > +             return ret;
> > > +
> > > +     file_path = expand_media_path(device_path);
> > > +     ret = EFI_CALL(efi_load_image(true, efi_root, file_path, NULL, 0,
> > > +                                   image_handle));
> > > +
> > > +     efi_free_pool(file_path);
> > > +
> > > +     return ret;
> > > +}
> >
> > We need to decide what we want here.  There were recently some patches from
> > Raymond [0] which piggybacked on your earlier eficonfig work.  I think the
> > last patch of this series hasn't been merged due to a test failing, but we
> > should fix it.
> > That patch has a different approach.  Everytime a disk appears it will add
> > a boot option if the default filepath is found and that's how we fixed the
> 
> efi_bootmgr_update_media_device_boot_option() automatically adds the
> boot option if the device handle installed EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> but it does not check whether the default file path is found or not.
> When the user selects the automatically created boot option,
> expand_media_path()
> is called and efibootmgr tries to boot with the default file.
> 

Ah correct.  On the function above though, we are we open coding a
different version of efi_open_protocol()?  Can't we call that instead of
efi_search_protocol()/efi_protocol_open()? 

> > behaviour of efibootmgr to adhere to the EFI spec.  This patch is doing the
> > opposite, trying to detect the BOOTAA64.EFI etc on the fly.  I think I
> > prefer the approach you have here, but we should end up with one solution
> > to solve both.
> 
> This HTTP Boot case is slightly different from the case where the user selects
> the automatically added boot option.
> In this case, user selects the URI device path boot option. The
> efibootmgr downloads the
> file, mount the image, and try to boot with the default file name on the fly.
> When the patch[0] is merged, it is possible to boot the downloaded iso
> image from the
> automatically added "blkmap" boot option, but I think efibootmgr needs
> to boot with
> the URI device path user selected that this series does, not boot
> from the "blkmap" boot option.
> 

Indeed, the used must still select the 'http://' boot option.  
It's been a while and I don't remember the eficonfig details.  Do you
remember why we decided to store the load options after scanning back then?  
IOW if we pick this up, can we also use it on the efibootmgr code and scan
all disks on the fly instead of adding boot options?

> >
> > > +
> > > +/**
> > > + * load_default_file_from_blk_dev() - load the default file
> > > + *
> > > + * @blk              pointer to the UCLASS_BLK udevice
> > > + * @handle:  pointer to handle for newly installed image
> > > + * Return:   status code
> > > + */
> > > +static efi_status_t load_default_file_from_blk_dev(struct udevice *blk,
> > > +                                                efi_handle_t *handle)
> > > +{
> > > +     efi_status_t ret;
> > > +     struct udevice *partition;
> > > +
> > > +     /* image that has no partition table but a file system */
> > > +     ret = try_load_default_file(blk, handle);
> > > +     if (ret == EFI_SUCCESS)
> > > +             return ret;
> > > +
> > > +     /* try the partitions */
> > > +     device_foreach_child(partition, blk) {
> > > +             enum uclass_id id;
> > > +
> > > +             id = device_get_uclass_id(partition);
> > > +             if (id != UCLASS_PARTITION)
> > > +                     continue;
> > > +
> > > +             ret = try_load_default_file(partition, handle);
> > > +             if (ret == EFI_SUCCESS)
> > > +                     return ret;
> > > +     }
> > > +
> > > +     return EFI_NOT_FOUND;
> > > +}
> > > +
> > > +/**
> > > + * 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)
> > > +{
> > > +     char *s;
> > > +     int uri_len;
> > > +     int image_size;
> > > +     efi_status_t ret;
> > > +     ulong image_addr;
> > > +
> > > +     s = env_get("loadaddr");
> > > +     if (!s) {
> > > +             log_err("Error: loadaddr is not set\n");
> > > +             return EFI_INVALID_PARAMETER;
> > > +     }
> > > +     image_addr = hextoul(s, NULL);
> > > +     image_size = wget_with_dns(image_addr, uridp->uri);
> > > +     if (image_size < 0)
> > > +             return EFI_INVALID_PARAMETER;
> > > +
> > > +     /*
> > > +      * If the file extension is ".iso" or ".img", mount it and try to load
> > > +      * the default file.
> >
> > Don't we have a better way to validate isos and efi apps instead of
> > the extension?  The efi is checked against a valid PE/COFF image so I guess
> > we'll really on the mount to fail for isos?
> 
> EDK2 reference implementation checks the file type with the following priority.
>  1) "Content-Type" header in HTTP response message
>  2) Filename Extensions
> Documentation is here[1].
> 
> Since "Content-Type" is not available in the current U-Boot wget, file extension
> is used to identify ISO images.

Ok fair enough, we can go back and improve this once lwip is merged I guess

> 
> [1] https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot#ram-disk-boot-from-http
> 

[...]

Thanks
/Ilias

Re: [PATCH v2 4/6] efi_loader: support boot from URI device path

[Masahisa Kojima]

On Fri, 15 Sept 2023 at 15:32, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Kojima-san,
>
> > > > +                                       efi_handle_t *image_handle)
> > > > +{
> > > > +     efi_status_t ret;
> > > > +     efi_handle_t bm_handle;
> > > > +     struct efi_handler *handler;
> > > > +     struct efi_device_path *file_path;
> > > > +     struct efi_device_path *device_path;
> > > > +
> > > > +     if (dev_tag_get_ptr(dev, DM_TAG_EFI, (void **)&bm_handle)) {
> > > > +             log_warning("DM_TAG_EFI not found\n");
> > > > +             return EFI_INVALID_PARAMETER;
> > > > +     }
> > > > +
> > > > +     ret = efi_search_protocol(bm_handle,
> > > > +                               &efi_simple_file_system_protocol_guid, &handler);
> > > > +     if (ret != EFI_SUCCESS)
> > > > +             return ret;
> > > > +
> > > > +     ret = efi_search_protocol(bm_handle, &efi_guid_device_path, &handler);
> > > > +     if (ret != EFI_SUCCESS)
> > > > +             return ret;
> > > > +
> > > > +     ret = efi_protocol_open(handler, (void **)&device_path, efi_root, NULL,
> > > > +                             EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> > > > +     if (ret != EFI_SUCCESS)
> > > > +             return ret;
> > > > +
> > > > +     file_path = expand_media_path(device_path);
> > > > +     ret = EFI_CALL(efi_load_image(true, efi_root, file_path, NULL, 0,
> > > > +                                   image_handle));
> > > > +
> > > > +     efi_free_pool(file_path);
> > > > +
> > > > +     return ret;
> > > > +}
> > >
> > > We need to decide what we want here.  There were recently some patches from
> > > Raymond [0] which piggybacked on your earlier eficonfig work.  I think the
> > > last patch of this series hasn't been merged due to a test failing, but we
> > > should fix it.
> > > That patch has a different approach.  Everytime a disk appears it will add
> > > a boot option if the default filepath is found and that's how we fixed the
> >
> > efi_bootmgr_update_media_device_boot_option() automatically adds the
> > boot option if the device handle installed EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> > but it does not check whether the default file path is found or not.
> > When the user selects the automatically created boot option,
> > expand_media_path()
> > is called and efibootmgr tries to boot with the default file.
> >
>
> Ah correct.  On the function above though, we are we open coding a
> different version of efi_open_protocol()?  Can't we call that instead of
> efi_search_protocol()/efi_protocol_open()?

I will call efi_open_protocol() instead. Thanks.

>
> > > behaviour of efibootmgr to adhere to the EFI spec.  This patch is doing the
> > > opposite, trying to detect the BOOTAA64.EFI etc on the fly.  I think I
> > > prefer the approach you have here, but we should end up with one solution
> > > to solve both.
> >
> > This HTTP Boot case is slightly different from the case where the user selects
> > the automatically added boot option.
> > In this case, user selects the URI device path boot option. The
> > efibootmgr downloads the
> > file, mount the image, and try to boot with the default file name on the fly.
> > When the patch[0] is merged, it is possible to boot the downloaded iso
> > image from the
> > automatically added "blkmap" boot option, but I think efibootmgr needs
> > to boot with
> > the URI device path user selected that this series does, not boot
> > from the "blkmap" boot option.
> >
>
> Indeed, the used must still select the 'http://' boot option.
> It's been a while and I don't remember the eficonfig details.  Do you
> remember why we decided to store the load options after scanning back then?

We just followed the EDK2 reference implementation.

> IOW if we pick this up, can we also use it on the efibootmgr code and scan
> all disks on the fly instead of adding boot options?

Yes, it is possible, but I'm not sure scanning all the disks on the fly
is a good idea. For users, it is hard to control which device is selected
to boot.

Now come to think of it, we add the one load option for each disk when the
disk is detected, then try to boot with the default file by scanning
the default boot file
in the selected disk.

In current implementation, the following load options are automatically created.

Boot0000:
attributes: A-- (0x00000001)
  label: virtio 0:1
  file_path: /HD(1,GPT,b6b38698-f211-4d78-b208-f68e5523e588,0x800,0x100000)
  data:
    00000000: c1 ac c1 38 c0 9f f0 41 b9 01 fa 74 d6 d6 e4 de  ...8...A...t....
Boot0001:
attributes: A-- (0x00000001)
  label: virtio 0:2
  file_path: /HD(2,GPT,98533ff2-11d5-428c-b323-9afaf6b4abd8,0x100800,0x1122000)
  data:
    00000000: c1 ac c1 38 c0 9f f0 41 b9 01 fa 74 d6 d6 e4 de  ...8...A...t....

For this disk, only the block device "virtio 0" load option is enough,
we can search
for the default boot file on the fly. This is what EDK2 does.

Thanks,
Masahisa Kojima



>
> > >
> > > > +
> > > > +/**
> > > > + * load_default_file_from_blk_dev() - load the default file
> > > > + *
> > > > + * @blk              pointer to the UCLASS_BLK udevice
> > > > + * @handle:  pointer to handle for newly installed image
> > > > + * Return:   status code
> > > > + */
> > > > +static efi_status_t load_default_file_from_blk_dev(struct udevice *blk,
> > > > +                                                efi_handle_t *handle)
> > > > +{
> > > > +     efi_status_t ret;
> > > > +     struct udevice *partition;
> > > > +
> > > > +     /* image that has no partition table but a file system */
> > > > +     ret = try_load_default_file(blk, handle);
> > > > +     if (ret == EFI_SUCCESS)
> > > > +             return ret;
> > > > +
> > > > +     /* try the partitions */
> > > > +     device_foreach_child(partition, blk) {
> > > > +             enum uclass_id id;
> > > > +
> > > > +             id = device_get_uclass_id(partition);
> > > > +             if (id != UCLASS_PARTITION)
> > > > +                     continue;
> > > > +
> > > > +             ret = try_load_default_file(partition, handle);
> > > > +             if (ret == EFI_SUCCESS)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > > +     return EFI_NOT_FOUND;
> > > > +}
> > > > +
> > > > +/**
> > > > + * 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)
> > > > +{
> > > > +     char *s;
> > > > +     int uri_len;
> > > > +     int image_size;
> > > > +     efi_status_t ret;
> > > > +     ulong image_addr;
> > > > +
> > > > +     s = env_get("loadaddr");
> > > > +     if (!s) {
> > > > +             log_err("Error: loadaddr is not set\n");
> > > > +             return EFI_INVALID_PARAMETER;
> > > > +     }
> > > > +     image_addr = hextoul(s, NULL);
> > > > +     image_size = wget_with_dns(image_addr, uridp->uri);
> > > > +     if (image_size < 0)
> > > > +             return EFI_INVALID_PARAMETER;
> > > > +
> > > > +     /*
> > > > +      * If the file extension is ".iso" or ".img", mount it and try to load
> > > > +      * the default file.
> > >
> > > Don't we have a better way to validate isos and efi apps instead of
> > > the extension?  The efi is checked against a valid PE/COFF image so I guess
> > > we'll really on the mount to fail for isos?
> >
> > EDK2 reference implementation checks the file type with the following priority.
> >  1) "Content-Type" header in HTTP response message
> >  2) Filename Extensions
> > Documentation is here[1].
> >
> > Since "Content-Type" is not available in the current U-Boot wget, file extension
> > is used to identify ISO images.
>
> Ok fair enough, we can go back and improve this once lwip is merged I guess
>
> >
> > [1] https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot#ram-disk-boot-from-http
> >
>
> [...]
>
> Thanks
> /Ilias

Re: [PATCH v2 4/6] efi_loader: support boot from URI device path

[Ilias Apalodimas]

Kojima-san

[...]

>
> We just followed the EDK2 reference implementation.
>
> > IOW if we pick this up, can we also use it on the efibootmgr code and scan
> > all disks on the fly instead of adding boot options?
>
> Yes, it is possible, but I'm not sure scanning all the disks on the fly
> is a good idea. For users, it is hard to control which device is selected
> to boot.
>
> Now come to think of it, we add the one load option for each disk when the
> disk is detected, then try to boot with the default file by scanning
> the default boot file
> in the selected disk.
>
> In current implementation, the following load options are automatically created.
>
> Boot0000:
> attributes: A-- (0x00000001)
>   label: virtio 0:1
>   file_path: /HD(1,GPT,b6b38698-f211-4d78-b208-f68e5523e588,0x800,0x100000)
>   data:
>     00000000: c1 ac c1 38 c0 9f f0 41 b9 01 fa 74 d6 d6 e4 de  ...8...A...t....
> Boot0001:
> attributes: A-- (0x00000001)
>   label: virtio 0:2
>   file_path: /HD(2,GPT,98533ff2-11d5-428c-b323-9afaf6b4abd8,0x100800,0x1122000)
>   data:
>     00000000: c1 ac c1 38 c0 9f f0 41 b9 01 fa 74 d6 d6 e4 de  ...8...A...t....

Yep, that's exactly what I was thinking as an alternative.  On the
automatically scan boot option, just search for '1234567' on the load
options.
Keep in mind that patch from Raymon is failing on an erofs selftest.
But when I was looking at it, I came to the conclusion, that due to
the EFI subsystem coming up earlier a print changes and that test gets
utterly confused.  IOW that patch seems fine, but you'll probably need
to adjust a selftest as well.

Thanks
/Ilias
>
> For this disk, only the block device "virtio 0" load option is enough,
> we can search
> for the default boot file on the fly. This is what EDK2 does.
>
> Thanks,
> Masahisa Kojima
>
>
>
> >
> > > >
> > > > > +
> > > > > +/**
> > > > > + * load_default_file_from_blk_dev() - load the default file
> > > > > + *
> > > > > + * @blk              pointer to the UCLASS_BLK udevice
> > > > > + * @handle:  pointer to handle for newly installed image
> > > > > + * Return:   status code
> > > > > + */
> > > > > +static efi_status_t load_default_file_from_blk_dev(struct udevice *blk,
> > > > > +                                                efi_handle_t *handle)
> > > > > +{
> > > > > +     efi_status_t ret;
> > > > > +     struct udevice *partition;
> > > > > +
> > > > > +     /* image that has no partition table but a file system */
> > > > > +     ret = try_load_default_file(blk, handle);
> > > > > +     if (ret == EFI_SUCCESS)
> > > > > +             return ret;
> > > > > +
> > > > > +     /* try the partitions */
> > > > > +     device_foreach_child(partition, blk) {
> > > > > +             enum uclass_id id;
> > > > > +
> > > > > +             id = device_get_uclass_id(partition);
> > > > > +             if (id != UCLASS_PARTITION)
> > > > > +                     continue;
> > > > > +
> > > > > +             ret = try_load_default_file(partition, handle);
> > > > > +             if (ret == EFI_SUCCESS)
> > > > > +                     return ret;
> > > > > +     }
> > > > > +
> > > > > +     return EFI_NOT_FOUND;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * 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)
> > > > > +{
> > > > > +     char *s;
> > > > > +     int uri_len;
> > > > > +     int image_size;
> > > > > +     efi_status_t ret;
> > > > > +     ulong image_addr;
> > > > > +
> > > > > +     s = env_get("loadaddr");
> > > > > +     if (!s) {
> > > > > +             log_err("Error: loadaddr is not set\n");
> > > > > +             return EFI_INVALID_PARAMETER;
> > > > > +     }
> > > > > +     image_addr = hextoul(s, NULL);
> > > > > +     image_size = wget_with_dns(image_addr, uridp->uri);
> > > > > +     if (image_size < 0)
> > > > > +             return EFI_INVALID_PARAMETER;
> > > > > +
> > > > > +     /*
> > > > > +      * If the file extension is ".iso" or ".img", mount it and try to load
> > > > > +      * the default file.
> > > >
> > > > Don't we have a better way to validate isos and efi apps instead of
> > > > the extension?  The efi is checked against a valid PE/COFF image so I guess
> > > > we'll really on the mount to fail for isos?
> > >
> > > EDK2 reference implementation checks the file type with the following priority.
> > >  1) "Content-Type" header in HTTP response message
> > >  2) Filename Extensions
> > > Documentation is here[1].
> > >
> > > Since "Content-Type" is not available in the current U-Boot wget, file extension
> > > is used to identify ISO images.
> >
> > Ok fair enough, we can go back and improve this once lwip is merged I guess
> >
> > >
> > > [1] https://github.com/tianocore/tianocore.github.io/wiki/HTTP-Boot#ram-disk-boot-from-http
> > >
> >
> > [...]
> >
> > Thanks
> > /Ilias