[PATCH v4 0/8] Add EFI HTTP boot support

[PATCH v4 0/8] 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
- uri device path support in eficonfig

[change log]
v3 -> v4
- patch#8 is added to simplify the bootmgr default boot process
- add function comments

v2 -> v3
- Patch#6 is added, reserve the whole ramdisk memory region
- remove .efi file extension check for PE-COFF image
- use "if IS_ENABLED(..)" as much as possible
- 1024 should be sizeof(net_boot_file_name)
- call net_set_state(NETLOOP_FAIL) when wget encounters error
- describe DNS ip address host name limitation in document

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 (8):
  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
  efi_loader: set EFI HTTP Boot download buffer as reserved
  cmd: efidebug: add uri device path
  doc: uefi: add HTTP Boot support
  efi_loader: create BlockIo device boot option

 cmd/efidebug.c                   |  50 +++++++
 doc/develop/uefi/uefi.rst        |  30 ++++
 drivers/block/Makefile           |   1 +
 drivers/block/blkmap.c           |  15 --
 drivers/block/blkmap_helper.c    |  53 +++++++
 include/blkmap.h                 |  29 ++++
 include/efi_loader.h             |   3 +
 include/net.h                    |  17 +++
 lib/efi_loader/efi_bootmgr.c     | 241 +++++++++++++++++++++++++++++--
 lib/efi_loader/efi_device_path.c |  20 +++
 lib/efi_loader/efi_dt_fixup.c    |   2 +-
 net/wget.c                       | 206 +++++++++++++++++++++++++-
 12 files changed, 628 insertions(+), 39 deletions(-)
 create mode 100644 drivers/block/blkmap_helper.c

-- 
2.34.1

[PATCH v4 1/8] 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>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
 net/wget.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 73 insertions(+), 7 deletions(-)

diff --git a/net/wget.c b/net/wget.c
index 2dbfeb1a1d..a48a8cb624 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,29 @@ 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 */
 
+static ulong wget_load_size;
+
+/**
+ * wget_init_max_size() - initialize maximum load size
+ *
+ * Return:	0 if success, -1 if fails
+ */
+static int wget_init_load_size(void)
+{
+	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;
+
+	return 0;
+}
+
 /**
  * store_block() - store block in memory
  * @src: source of data
@@ -63,10 +90,25 @@ 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);
+	if (IS_ENABLED(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) {
+			printf("\nwget error: ");
+			printf("trying to overwrite reserved memory...\n");
+			return -1;
+		}
+	}
+
+	ptr = map_sysmem(store_addr, len);
 	memcpy(ptr, src, len);
 	unmap_sysmem(ptr);
 
@@ -240,25 +282,39 @@ 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);
+					net_set_state(NETLOOP_FAIL);
+					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);
+					net_set_state(NETLOOP_FAIL);
+					return;
+				}
 			}
 		}
 	}
@@ -330,6 +386,7 @@ static void wget_handler(uchar *pkt, u16 dport,
 				len) != 0) {
 			wget_fail("wget: store error\n",
 				  tcp_seq_num, tcp_ack_num, action);
+			net_set_state(NETLOOP_FAIL);
 			return;
 		}
 
@@ -420,6 +477,15 @@ void wget_start(void)
 	debug_cond(DEBUG_WGET,
 		   "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
 
+	if (IS_ENABLED(CONFIG_LMB)) {
+		if (wget_init_load_size()) {
+			printf("\nwget error: ");
+			printf("trying to overwrite reserved memory...\n");
+			net_set_state(NETLOOP_FAIL);
+			return;
+		}
+	}
+
 	net_set_timeout_handler(wget_timeout, wget_timeout_handler);
 	tcp_set_tcp_handler(wget_handler);
 
-- 
2.34.1

[PATCH v4 2/8] 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 |  9 +++++++++
 net/wget.c    | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/include/net.h b/include/net.h
index e254df7d7f..57889d8b7a 100644
--- a/include/net.h
+++ b/include/net.h
@@ -926,4 +926,13 @@ void eth_set_enable_bootdevs(bool enable);
 static inline void eth_set_enable_bootdevs(bool enable) {}
 #endif
 
+/**
+ * wget_with_dns() - runs dns host IP address resulution before wget
+ *
+ * @dst_addr:	destination address to download the file
+ * @uri:	uri string of target file of wget
+ * Return:	downloaded file size, negative if failed
+ */
+int wget_with_dns(ulong dst_addr, char *uri);
+
 #endif /* __NET_H__ */
diff --git a/net/wget.c b/net/wget.c
index a48a8cb624..4801e28eb9 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;
 
@@ -504,3 +505,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, sizeof(net_boot_file_name));
+	strlcat(net_boot_file_name, ":/", sizeof(net_boot_file_name)); /* append '/' which is removed by strsep() */
+	strlcat(net_boot_file_name, file_name, sizeof(net_boot_file_name));
+	image_load_addr = dst_addr;
+	ret = net_loop(WGET);
+
+out:
+	free(str_copy);
+
+	return ret;
+}
+#endif
-- 
2.34.1

[PATCH v4 3/8] 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>
Reviewed-by: Simon Glass <sjg@chromium.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 v4 4/8] 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 | 189 +++++++++++++++++++++++++++++++++++
 1 file changed, 189 insertions(+)

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index a40762c74c..605be5041e 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,182 @@ out:
 	return ret;
 }
 
+/**
+ * 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 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 **)&handle)) {
+		log_warning("DM_TAG_EFI not found\n");
+		return EFI_INVALID_PARAMETER;
+	}
+
+	ret = efi_search_protocol(handle,
+				  &efi_simple_file_system_protocol_guid, &handler);
+	if (ret != EFI_SUCCESS)
+		return ret;
+
+	ret = EFI_CALL(bs->open_protocol(handle, &efi_guid_device_path,
+					 (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 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 (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
+		efi_handle_t mem_handle = NULL;
+		struct efi_device_path *file_path = NULL;
+
+		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;
+}
+
 /**
  * try_load_entry() - try to load image for boot option
  *
@@ -211,6 +391,15 @@ 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);
+		} else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
+			if (IS_ENABLED(CONFIG_BLKMAP) &&
+			    IS_ENABLED(CONFIG_CMD_WGET) &&
+			    IS_ENABLED(CONFIG_CMD_DNS)) {
+				ret = try_load_from_uri_path(
+					(struct efi_device_path_uri *)
+						lo.file_path,
+					lo.label, handle);
+			}
 		} else {
 			file_path = expand_media_path(lo.file_path);
 			ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
-- 
2.34.1

[PATCH v4 5/8] efi_loader: set EFI HTTP Boot download buffer as reserved

[Masahisa Kojima]

The buffer used to download the ISO image file must be
reserved to avoid the unintended access to the image.

For PE-COFF file case, this memory reservation is done
in LoadImage Boot Service.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 include/efi_loader.h          | 2 ++
 lib/efi_loader/efi_bootmgr.c  | 5 +++++
 lib/efi_loader/efi_dt_fixup.c | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index 4a29ddaef4..c4207edc91 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -554,6 +554,8 @@ void efi_runtime_detach(void);
 /* efi_convert_pointer() - convert pointer to virtual address */
 efi_status_t EFIAPI efi_convert_pointer(efi_uintn_t debug_disposition,
 					void **address);
+/* add reserved memory to memory map */
+void efi_reserve_memory(u64 addr, u64 size, bool nomap);
 /* Carve out DT reserved memory ranges */
 void efi_carve_out_dt_rsv(void *fdt);
 /* Purge unused kaslr-seed */
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 605be5041e..4991056946 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -326,6 +326,11 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
 			return EFI_INVALID_PARAMETER;
 
 		ret = load_default_file_from_blk_dev(blk, handle);
+		if (ret != EFI_SUCCESS)
+			return ret;
+
+		/* whole ramdisk must be reserved */
+		efi_reserve_memory(image_addr, image_size, true);
 	} else if (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
 		efi_handle_t mem_handle = NULL;
 		struct efi_device_path *file_path = NULL;
diff --git a/lib/efi_loader/efi_dt_fixup.c b/lib/efi_loader/efi_dt_fixup.c
index 838023c78f..edc515b9ff 100644
--- a/lib/efi_loader/efi_dt_fixup.c
+++ b/lib/efi_loader/efi_dt_fixup.c
@@ -22,7 +22,7 @@ const efi_guid_t efi_guid_dt_fixup_protocol = EFI_DT_FIXUP_PROTOCOL_GUID;
  * @nomap:	indicates that the memory range shall not be accessed by the
  *		UEFI payload
  */
-static void efi_reserve_memory(u64 addr, u64 size, bool nomap)
+void efi_reserve_memory(u64 addr, u64 size, bool nomap)
 {
 	int type;
 	efi_uintn_t ret;
-- 
2.34.1

[PATCH v4 6/8] 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  |  8 ++++++
 net/wget.c     | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 130 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 57889d8b7a..c748974573 100644
--- a/include/net.h
+++ b/include/net.h
@@ -935,4 +935,12 @@ static inline void eth_set_enable_bootdevs(bool enable) {}
  */
 int wget_with_dns(ulong dst_addr, char *uri);
 
+/**
+ * wget_validate_uri() - varidate the uri
+ *
+ * @uri:	uri string of target file of wget
+ * Return:	true if uri is valid, false if uri is invalid
+ */
+bool wget_validate_uri(char *uri);
+
 #endif /* __NET_H__ */
diff --git a/net/wget.c b/net/wget.c
index 4801e28eb9..6a4d22be32 100644
--- a/net/wget.c
+++ b/net/wget.c
@@ -558,3 +558,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 v4 7/8] 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 | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
index a7a41f2fac..65eea89265 100644
--- a/doc/develop/uefi/uefi.rst
+++ b/doc/develop/uefi/uefi.rst
@@ -594,6 +594,36 @@ 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 dns, 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 is PE-COFF image, load the downloaded file and
+start it.
+
+There is a limitation that current implementation tries to resolve
+the IP address as a host name. If the uri is like "http://192.168.1.1/foobar",
+the dns process tries to resolve the host "192.168.1.1" and it will
+end up with "host not found".
+
+We need to preset the "httpserverip" environment variable to proceed the wget::
+
+    setenv httpserverip 192.168.1.1
+
 Executing the built in hello world application
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 
-- 
2.34.1

[PATCH v4 8/8] efi_loader: create BlockIo device boot option

[Masahisa Kojima]

Current efibootmgr automatically creates the boot options
of all the disks and partitions installing
SIMPLE_FILE_SYSTEM_PROTOCOL. These boot options are created
to load and start the default file(e.g. EFI/BOOT/BOOTAA64.EFI).

Now efibootmgr can scan the BlockIo device and try to boot
with the default file on the fly, this commit creates the
boot options only for the BlockIo devices exluding the logical
partition.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 include/efi_loader.h             |  1 +
 lib/efi_loader/efi_bootmgr.c     | 47 +++++++++++++++++++++-----------
 lib/efi_loader/efi_device_path.c | 20 ++++++++++++++
 3 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index c4207edc91..cc292f0553 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -932,6 +932,7 @@ struct efi_device_path *efi_dp_from_lo(struct efi_load_option *lo,
 struct efi_device_path *efi_dp_concat(const struct efi_device_path *dp1,
 				      const struct efi_device_path *dp2);
 struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path);
+struct efi_device_path *efi_search_file_path_dp_node(struct efi_device_path *device_path);
 efi_status_t efi_deserialize_load_option(struct efi_load_option *lo, u8 *data,
 					 efi_uintn_t *size);
 unsigned long efi_serialize_load_option(struct efi_load_option *lo, u8 **data);
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 4991056946..b55d651d1b 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -387,7 +387,6 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
 	}
 
 	if (lo.attributes & LOAD_OPTION_ACTIVE) {
-		struct efi_device_path *file_path;
 		u32 attributes;
 
 		log_debug("trying to load \"%ls\" from %pD\n", lo.label,
@@ -405,11 +404,14 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
 						lo.file_path,
 					lo.label, handle);
 			}
-		} else {
-			file_path = expand_media_path(lo.file_path);
-			ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
+		} else if (efi_search_file_path_dp_node(lo.file_path)) {
+			ret = EFI_CALL(efi_load_image(true, efi_root, lo.file_path,
 						      NULL, 0, handle));
-			efi_free_pool(file_path);
+		} else {
+			efi_handle_t h;
+
+			h = efi_dp_find_obj(lo.file_path, &efi_block_io_guid, NULL);
+			ret = load_default_file_from_blk_dev(h->dev, handle);
 		}
 		if (ret != EFI_SUCCESS) {
 			log_warning("Loading %ls '%ls' failed\n",
@@ -549,13 +551,13 @@ error:
  */
 static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boot_option *opt,
 						      efi_handle_t *volume_handles,
-						      efi_status_t count)
+						      efi_uintn_t *count)
 {
-	u32 i;
+	u32 i, num = 0;
 	struct efi_handler *handler;
 	efi_status_t ret = EFI_SUCCESS;
 
-	for (i = 0; i < count; i++) {
+	for (i = 0; i < *count; i++) {
 		u16 *p;
 		u16 dev_name[BOOTMENU_DEVICE_NAME_MAX];
 		char *optional_data;
@@ -563,6 +565,16 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
 		char buf[BOOTMENU_DEVICE_NAME_MAX];
 		struct efi_device_path *device_path;
 		struct efi_device_path *short_dp;
+		struct efi_block_io *blkio;
+
+		ret = efi_search_protocol(volume_handles[i], &efi_block_io_guid, &handler);
+		blkio = handler->protocol_interface;
+		/*
+		 * The logical partition is excluded since the bootmgr tries to
+		 * boot with the default file by scanning the default file on the fly.
+		 */
+		if (blkio->media->logical_partition)
+			continue;
 
 		ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
 		if (ret != EFI_SUCCESS)
@@ -596,16 +608,18 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
 		 * to store guid, instead of realloc the load_option.
 		 */
 		lo.optional_data = "1234567";
-		opt[i].size = efi_serialize_load_option(&lo, (u8 **)&opt[i].lo);
-		if (!opt[i].size) {
+		opt[num].size = efi_serialize_load_option(&lo, (u8 **)&opt[num].lo);
+		if (!opt[num].size) {
 			ret = EFI_OUT_OF_RESOURCES;
 			goto out;
 		}
 		/* set the guid */
-		optional_data = (char *)opt[i].lo + (opt[i].size - u16_strsize(u"1234567"));
+		optional_data = (char *)opt[num].lo + (opt[num].size - u16_strsize(u"1234567"));
 		memcpy(optional_data, &efi_guid_bootmenu_auto_generated, sizeof(efi_guid_t));
+		num++;
 	}
 
+	*count = num;
 out:
 	return ret;
 }
@@ -835,8 +849,7 @@ efi_status_t efi_bootmgr_delete_boot_option(u16 boot_index)
 /**
  * efi_bootmgr_update_media_device_boot_option() - generate the media device boot option
  *
- * This function enumerates all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
- * and generate the bootmenu entries.
+ * This function enumerates all BlockIo devices and add the boot option for it.
  * This function also provide the BOOT#### variable maintenance for
  * the media device entries.
  * - Automatically create the BOOT#### variable for the newly detected device,
@@ -856,7 +869,7 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
 	struct eficonfig_media_boot_option *opt = NULL;
 
 	ret = efi_locate_handle_buffer_int(BY_PROTOCOL,
-					   &efi_simple_file_system_protocol_guid,
+					   &efi_block_io_guid,
 					   NULL, &count,
 					   (efi_handle_t **)&volume_handles);
 	if (ret != EFI_SUCCESS)
@@ -868,8 +881,7 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
 		goto out;
 	}
 
-	/* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */
-	ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);
+	ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, &count);
 	if (ret != EFI_SUCCESS)
 		goto out;
 
@@ -888,6 +900,9 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void)
 		u32 boot_index;
 		u16 var_name[9];
 
+		if (!opt[i].size)
+			continue;
+
 		if (!opt[i].exist) {
 			ret = efi_bootmgr_get_unused_bootoption(var_name, sizeof(var_name),
 								&boot_index);
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index ed7214f3a3..a34ef3da68 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -1219,3 +1219,23 @@ struct efi_device_path *search_gpt_dp_node(struct efi_device_path *device_path)
 
 	return NULL;
 }
+
+/**
+ * efi_search_file_path_dp_node() - search file_path device path node
+ *
+ * @device_path:	device path
+ *
+ * Return:	pointer to the DEVICE_PATH_SUB_TYPE_FILE_PATH device path node
+ */
+struct efi_device_path *efi_search_file_path_dp_node(struct efi_device_path *device_path)
+{
+	struct efi_device_path *dp = device_path;
+
+	while (dp) {
+		if (EFI_DP_TYPE(dp, MEDIA_DEVICE, FILE_PATH))
+			return dp;
+		dp = efi_dp_next(dp);
+	}
+
+	return NULL;
+}
-- 
2.34.1

Re: [PATCH v4 1/8] net: wget: prevent overwriting reserved memory

[Ilias Apalodimas]

On Fri, Sep 22, 2023 at 04:11:12PM +0900, Masahisa Kojima 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>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
>  net/wget.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 73 insertions(+), 7 deletions(-)
>
> diff --git a/net/wget.c b/net/wget.c
> index 2dbfeb1a1d..a48a8cb624 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,29 @@ 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 */
>
> +static ulong wget_load_size;
> +
> +/**
> + * wget_init_max_size() - initialize maximum load size
> + *
> + * Return:	0 if success, -1 if fails
> + */
> +static int wget_init_load_size(void)
> +{
> +	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;
> +
> +	return 0;
> +}
> +
>  /**
>   * store_block() - store block in memory
>   * @src: source of data
> @@ -63,10 +90,25 @@ 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);
> +	if (IS_ENABLED(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) {
> +			printf("\nwget error: ");
> +			printf("trying to overwrite reserved memory...\n");
> +			return -1;
> +		}
> +	}
> +
> +	ptr = map_sysmem(store_addr, len);
>  	memcpy(ptr, src, len);
>  	unmap_sysmem(ptr);
>
> @@ -240,25 +282,39 @@ 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);
> +					net_set_state(NETLOOP_FAIL);
> +					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);
> +					net_set_state(NETLOOP_FAIL);
> +					return;
> +				}
>  			}
>  		}
>  	}
> @@ -330,6 +386,7 @@ static void wget_handler(uchar *pkt, u16 dport,
>  				len) != 0) {
>  			wget_fail("wget: store error\n",
>  				  tcp_seq_num, tcp_ack_num, action);
> +			net_set_state(NETLOOP_FAIL);
>  			return;
>  		}
>
> @@ -420,6 +477,15 @@ void wget_start(void)
>  	debug_cond(DEBUG_WGET,
>  		   "\nwget:Load address: 0x%lx\nLoading: *\b", image_load_addr);
>
> +	if (IS_ENABLED(CONFIG_LMB)) {
> +		if (wget_init_load_size()) {
> +			printf("\nwget error: ");
> +			printf("trying to overwrite reserved memory...\n");
> +			net_set_state(NETLOOP_FAIL);
> +			return;
> +		}
> +	}
> +
>  	net_set_timeout_handler(wget_timeout, wget_timeout_handler);
>  	tcp_set_tcp_handler(wget_handler);
>
> --
> 2.34.1
>

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v4 2/8] net: wget: add wget with dns utility function

[Ilias Apalodimas]

On Fri, Sep 22, 2023 at 04:11:13PM +0900, Masahisa Kojima wrote:
> 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 |  9 +++++++++
>  net/wget.c    | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 63 insertions(+)
>
> diff --git a/include/net.h b/include/net.h
> index e254df7d7f..57889d8b7a 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -926,4 +926,13 @@ void eth_set_enable_bootdevs(bool enable);
>  static inline void eth_set_enable_bootdevs(bool enable) {}
>  #endif
>
> +/**
> + * wget_with_dns() - runs dns host IP address resulution before wget
> + *
> + * @dst_addr:	destination address to download the file
> + * @uri:	uri string of target file of wget
> + * Return:	downloaded file size, negative if failed
> + */
> +int wget_with_dns(ulong dst_addr, char *uri);
> +
>  #endif /* __NET_H__ */
> diff --git a/net/wget.c b/net/wget.c
> index a48a8cb624..4801e28eb9 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;
>
> @@ -504,3 +505,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, sizeof(net_boot_file_name));
> +	strlcat(net_boot_file_name, ":/", sizeof(net_boot_file_name)); /* append '/' which is removed by strsep() */
> +	strlcat(net_boot_file_name, file_name, sizeof(net_boot_file_name));
> +	image_load_addr = dst_addr;
> +	ret = net_loop(WGET);
> +
> +out:
> +	free(str_copy);
> +
> +	return ret;
> +}
> +#endif
> --
> 2.34.1
>

This can be done better when we integrate LWIP but for now

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v4 3/8] blk: blkmap: add ramdisk creation utility function

[Ilias Apalodimas]

On Fri, Sep 22, 2023 at 04:11:14PM +0900, Masahisa Kojima 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>
> Reviewed-by: Simon Glass <sjg@chromium.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
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v4 7/8] doc: uefi: add HTTP Boot support

[Ilias Apalodimas]

On Fri, Sep 22, 2023 at 04:11:18PM +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 | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
> index a7a41f2fac..65eea89265 100644
> --- a/doc/develop/uefi/uefi.rst
> +++ b/doc/develop/uefi/uefi.rst
> @@ -594,6 +594,36 @@ 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 dns, 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 is PE-COFF image, load the downloaded file and
> +start it.
> +
> +There is a limitation that current implementation tries to resolve

Remove the 'There is a limitation that', use The current implementation ...

> +the IP address as a host name. If the uri is like "http://192.168.1.1/foobar",
> +the dns process tries to resolve the host "192.168.1.1" and it will
> +end up with "host not found".
> +
> +We need to preset the "httpserverip" environment variable to proceed the wget::
> +
> +    setenv httpserverip 192.168.1.1
> +
>  Executing the built in hello world application
>  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> --
> 2.34.1
>

Other than the nit above
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v4 4/8] efi_loader: support boot from URI device path

[Ilias Apalodimas]

On Fri, Sep 22, 2023 at 04:11:15PM +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 | 189 +++++++++++++++++++++++++++++++++++
>  1 file changed, 189 insertions(+)
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a40762c74c..605be5041e 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,182 @@ out:
>  	return ret;
>  }
>
> +/**
> + * 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)
> +{

Maybe I misunderstood you on the previous series.  Wasn't the plan to merge
the patch that adds boot options automatically when a disk is scanned? This
code could only look for a boot option with '1234567' in its load options
instead of rescanning all devices with the EFI_SIMPLE_FILE_SYSTEM_PROTOCO
installed

> +	efi_status_t ret;
> +	efi_handle_t 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 **)&handle)) {
> +		log_warning("DM_TAG_EFI not found\n");
> +		return EFI_INVALID_PARAMETER;
> +	}
> +
> +	ret = efi_search_protocol(handle,
> +				  &efi_simple_file_system_protocol_guid, &handler);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	ret = EFI_CALL(bs->open_protocol(handle, &efi_guid_device_path,
> +					 (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 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 (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
> +		efi_handle_t mem_handle = NULL;
> +		struct efi_device_path *file_path = NULL;
> +
> +		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;
> +}
> +
>  /**
>   * try_load_entry() - try to load image for boot option
>   *
> @@ -211,6 +391,15 @@ 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);
> +		} else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
> +			if (IS_ENABLED(CONFIG_BLKMAP) &&
> +			    IS_ENABLED(CONFIG_CMD_WGET) &&
> +			    IS_ENABLED(CONFIG_CMD_DNS)) {
> +				ret = try_load_from_uri_path(
> +					(struct efi_device_path_uri *)
> +						lo.file_path,
> +					lo.label, handle);
> +			}

is ret initialized in this function?  Otherwise we need to initialize it if
one of the Kconfig options are disabled

>  		} else {
>  			file_path = expand_media_path(lo.file_path);
>  			ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> --
> 2.34.1
>

Thanks
/Ilias

Re: [PATCH v4 5/8] efi_loader: set EFI HTTP Boot download buffer as reserved

[Ilias Apalodimas]

Kojima-san,

[...]
>  /* Carve out DT reserved memory ranges */
>  void efi_carve_out_dt_rsv(void *fdt);
>  /* Purge unused kaslr-seed */
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 605be5041e..4991056946 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -326,6 +326,11 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
>  			return EFI_INVALID_PARAMETER;
>
>  		ret = load_default_file_from_blk_dev(blk, handle);
> +		if (ret != EFI_SUCCESS)
> +			return ret;
> +
> +		/* whole ramdisk must be reserved */
> +		efi_reserve_memory(image_addr, image_size, true);

Why is this a different patch though?
My concern is code duplication when we add similar functionality in
eficonfig.  Isn't there a better place to handle the memory reservation?

[...]

Thanks
/Ilias

Re: [PATCH v4 4/8] efi_loader: support boot from URI device path

[Masahisa Kojima]

Hi Ilias,

On Mon, 25 Sept 2023 at 21:37, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, Sep 22, 2023 at 04:11:15PM +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 | 189 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 189 insertions(+)
> >
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index a40762c74c..605be5041e 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,182 @@ out:
> >       return ret;
> >  }
> >
> > +/**
> > + * 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)
> > +{
>
> Maybe I misunderstood you on the previous series.  Wasn't the plan to merge
> the patch that adds boot options automatically when a disk is scanned? This

Adding the boot option automatically when a disk is scanned was sent
separately[1] from this series
since this feature is the matter of the timing of automatic boot
option creation and not directly related
to EFI HTTP Boot.
I also included the fixes of the efi_secboot python test failure.
Sorry for confusing you.

[1] https://patchwork.ozlabs.org/project/uboot/patch/20230920084121.1248590-1-masahisa.kojima@linaro.org/

> code could only look for a boot option with '1234567' in its load options
> instead of rescanning all devices with the EFI_SIMPLE_FILE_SYSTEM_PROTOCO
> installed

Now we can scan and load the default file(EFI/BOOT/BOOTAA64.EFI) on the fly with
load_default_file_from_blk_dev() function.
The patch #8 of this series "efi_loader: create BlockIo device boot
option" create the boot option
for the block device excluding the logical partition,

>
> > +     efi_status_t ret;
> > +     efi_handle_t 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 **)&handle)) {
> > +             log_warning("DM_TAG_EFI not found\n");
> > +             return EFI_INVALID_PARAMETER;
> > +     }
> > +
> > +     ret = efi_search_protocol(handle,
> > +                               &efi_simple_file_system_protocol_guid, &handler);
> > +     if (ret != EFI_SUCCESS)
> > +             return ret;
> > +
> > +     ret = EFI_CALL(bs->open_protocol(handle, &efi_guid_device_path,
> > +                                      (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 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 (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
> > +             efi_handle_t mem_handle = NULL;
> > +             struct efi_device_path *file_path = NULL;
> > +
> > +             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;
> > +}
> > +
> >  /**
> >   * try_load_entry() - try to load image for boot option
> >   *
> > @@ -211,6 +391,15 @@ 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);
> > +             } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
> > +                     if (IS_ENABLED(CONFIG_BLKMAP) &&
> > +                         IS_ENABLED(CONFIG_CMD_WGET) &&
> > +                         IS_ENABLED(CONFIG_CMD_DNS)) {
> > +                             ret = try_load_from_uri_path(
> > +                                     (struct efi_device_path_uri *)
> > +                                             lo.file_path,
> > +                                     lo.label, handle);
> > +                     }
>
> is ret initialized in this function?  Otherwise we need to initialize it if
> one of the Kconfig options are disabled

Thank you, I will add an 'else' statement to set the error here.

Thanks,
Masahisa Kojima

>
> >               } else {
> >                       file_path = expand_media_path(lo.file_path);
> >                       ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > --
> > 2.34.1
> >
>
> Thanks
> /Ilias

Re: [PATCH v4 7/8] doc: uefi: add HTTP Boot support

[Masahisa Kojima]

On Mon, 25 Sept 2023 at 21:12, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, Sep 22, 2023 at 04:11:18PM +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 | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> >
> > diff --git a/doc/develop/uefi/uefi.rst b/doc/develop/uefi/uefi.rst
> > index a7a41f2fac..65eea89265 100644
> > --- a/doc/develop/uefi/uefi.rst
> > +++ b/doc/develop/uefi/uefi.rst
> > @@ -594,6 +594,36 @@ 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 dns, 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 is PE-COFF image, load the downloaded file and
> > +start it.
> > +
> > +There is a limitation that current implementation tries to resolve
>
> Remove the 'There is a limitation that', use The current implementation ...

OK.

>
> > +the IP address as a host name. If the uri is like "http://192.168.1.1/foobar",
> > +the dns process tries to resolve the host "192.168.1.1" and it will
> > +end up with "host not found".
> > +
> > +We need to preset the "httpserverip" environment variable to proceed the wget::
> > +
> > +    setenv httpserverip 192.168.1.1
> > +
> >  Executing the built in hello world application
> >  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > --
> > 2.34.1
> >
>
> Other than the nit above
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Thanks,
Masahisa Kojima

>
>

Re: [PATCH v4 5/8] efi_loader: set EFI HTTP Boot download buffer as reserved

[Masahisa Kojima]

Hi Ilias,

On Mon, 25 Sept 2023 at 21:46, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Kojima-san,
>
> [...]
> >  /* Carve out DT reserved memory ranges */
> >  void efi_carve_out_dt_rsv(void *fdt);
> >  /* Purge unused kaslr-seed */
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 605be5041e..4991056946 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -326,6 +326,11 @@ static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> >                       return EFI_INVALID_PARAMETER;
> >
> >               ret = load_default_file_from_blk_dev(blk, handle);
> > +             if (ret != EFI_SUCCESS)
> > +                     return ret;
> > +
> > +             /* whole ramdisk must be reserved */
> > +             efi_reserve_memory(image_addr, image_size, true);
>
> Why is this a different patch though?

No special reason, I will merge this into #4 "efi_loader: support boot
from URI device path" patch.

> My concern is code duplication when we add similar functionality in
> eficonfig.  Isn't there a better place to handle the memory reservation?

I think eficonfig will only provide add/edit/delete URI boot option,
efibootmgr is
responsible for handling the URI device path and reserving the memory.
So there will not be code duplication.

Thanks,
Masahisa Kojima

>
> [...]
>
> Thanks
> /Ilias
>

Re: [PATCH v4 4/8] efi_loader: support boot from URI device path

[Maxim Uvarov]

On Tue, 26 Sept 2023 at 09:01, Masahisa Kojima <masahisa.kojima@linaro.org>
wrote:

> Hi Ilias,
>
> On Mon, 25 Sept 2023 at 21:37, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Fri, Sep 22, 2023 at 04:11:15PM +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 | 189 +++++++++++++++++++++++++++++++++++
> > >  1 file changed, 189 insertions(+)
> > >
> > > diff --git a/lib/efi_loader/efi_bootmgr.c
> b/lib/efi_loader/efi_bootmgr.c
> > > index a40762c74c..605be5041e 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,182 @@ out:
> > >       return ret;
> > >  }
> > >
> > > +/**
> > > + * 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)
> > > +{
> >
> > Maybe I misunderstood you on the previous series.  Wasn't the plan to
> merge
> > the patch that adds boot options automatically when a disk is scanned?
> This
>
> Adding the boot option automatically when a disk is scanned was sent
> separately[1] from this series
> since this feature is the matter of the timing of automatic boot
> option creation and not directly related
> to EFI HTTP Boot.
> I also included the fixes of the efi_secboot python test failure.
> Sorry for confusing you.
>
> [1]
> https://patchwork.ozlabs.org/project/uboot/patch/20230920084121.1248590-1-masahisa.kojima@linaro.org/
>
> > code could only look for a boot option with '1234567' in its load options
> > instead of rescanning all devices with the EFI_SIMPLE_FILE_SYSTEM_PROTOCO
> > installed
>
> Now we can scan and load the default file(EFI/BOOT/BOOTAA64.EFI) on the
> fly with
> load_default_file_from_blk_dev() function.
> The patch #8 of this series "efi_loader: create BlockIo device boot
> option" create the boot option
> for the block device excluding the logical partition,
>
> >
> > > +     efi_status_t ret;
> > > +     efi_handle_t 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 **)&handle)) {
> > > +             log_warning("DM_TAG_EFI not found\n");
> > > +             return EFI_INVALID_PARAMETER;
> > > +     }
> > > +
> > > +     ret = efi_search_protocol(handle,
> > > +                               &efi_simple_file_system_protocol_guid,
> &handler);
> > > +     if (ret != EFI_SUCCESS)
> > > +             return ret;
> > > +
> > > +     ret = EFI_CALL(bs->open_protocol(handle, &efi_guid_device_path,
> > > +                                      (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)
>

Maybe this comment is not related to this current commit, but on loading
http file we do not set maximum size to download. I.e. if the image will be
too big it will cause segfault I think.


> > > +{
> > > +     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;
>

maybe something like env_get_hex("filesize", 0); here?
Size is potentially limited with half of int type here.


> > > +
> > > +     /*
> > > +      * If the file extension is ".iso" or ".img", mount it and try
> to load
> > > +      * the default file.
> > > +      * If the file is 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)) {
>

Is it possible to check magic numbers rather than naming?
I see that at least .iso has one:
https://en.wikipedia.org/wiki/List_of_file_signatures


> > > +             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 (efi_check_pe((void *)image_addr, image_size, NULL) ==
> EFI_SUCCESS) {
> > > +             efi_handle_t mem_handle = NULL;
> > > +             struct efi_device_path *file_path = NULL;
>

no need to set file_path to NULL

> > > +
> > > +             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;
>
EFI_UNSUPPORTED or EFI_NO_MEDIA looks more suitable here.


> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  /**
> > >   * try_load_entry() - try to load image for boot option
> > >   *
> > > @@ -211,6 +391,15 @@ 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);
> > > +             } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE,
> MSG_URI)) {
> > > +                     if (IS_ENABLED(CONFIG_BLKMAP) &&
> > > +                         IS_ENABLED(CONFIG_CMD_WGET) &&
> > > +                         IS_ENABLED(CONFIG_CMD_DNS)) {
>

This looks like too many Kconfig options. Maybe there is a way to make some
function empty and let LTO do size optimization.


> > > +                             ret = try_load_from_uri_path(
> > > +                                     (struct efi_device_path_uri *)
> > > +                                             lo.file_path,
> > > +                                     lo.label, handle);
> > > +                     }
> >
> > is ret initialized in this function?  Otherwise we need to initialize it
> if
> > one of the Kconfig options are disabled
>
> Thank you, I will add an 'else' statement to set the error here.
>
> Thanks,
> Masahisa Kojima
>
> >
> > >               } else {
> > >                       file_path = expand_media_path(lo.file_path);
> > >                       ret = EFI_CALL(efi_load_image(true, efi_root,
> file_path,
> > > --
> > > 2.34.1
> > >
> >
> > Thanks
> > /Ilias
>

Re: [PATCH v4 4/8] efi_loader: support boot from URI device path

[Masahisa Kojima]

Hi Maxim,

On Thu, 28 Sept 2023 at 20:35, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
>
>
> On Tue, 26 Sept 2023 at 09:01, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>>
>> Hi Ilias,
>>
>> On Mon, 25 Sept 2023 at 21:37, Ilias Apalodimas
>> <ilias.apalodimas@linaro.org> wrote:
>> >
>> > On Fri, Sep 22, 2023 at 04:11:15PM +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 | 189 +++++++++++++++++++++++++++++++++++
>> > >  1 file changed, 189 insertions(+)
>> > >
>> > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>> > > index a40762c74c..605be5041e 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,182 @@ out:
>> > >       return ret;
>> > >  }
>> > >
>> > > +/**
>> > > + * 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)
>> > > +{
>> >
>> > Maybe I misunderstood you on the previous series.  Wasn't the plan to merge
>> > the patch that adds boot options automatically when a disk is scanned? This
>>
>> Adding the boot option automatically when a disk is scanned was sent
>> separately[1] from this series
>> since this feature is the matter of the timing of automatic boot
>> option creation and not directly related
>> to EFI HTTP Boot.
>> I also included the fixes of the efi_secboot python test failure.
>> Sorry for confusing you.
>>
>> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230920084121.1248590-1-masahisa.kojima@linaro.org/
>>
>> > code could only look for a boot option with '1234567' in its load options
>> > instead of rescanning all devices with the EFI_SIMPLE_FILE_SYSTEM_PROTOCO
>> > installed
>>
>> Now we can scan and load the default file(EFI/BOOT/BOOTAA64.EFI) on the fly with
>> load_default_file_from_blk_dev() function.
>> The patch #8 of this series "efi_loader: create BlockIo device boot
>> option" create the boot option
>> for the block device excluding the logical partition,
>>
>> >
>> > > +     efi_status_t ret;
>> > > +     efi_handle_t 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 **)&handle)) {
>> > > +             log_warning("DM_TAG_EFI not found\n");
>> > > +             return EFI_INVALID_PARAMETER;
>> > > +     }
>> > > +
>> > > +     ret = efi_search_protocol(handle,
>> > > +                               &efi_simple_file_system_protocol_guid, &handler);
>> > > +     if (ret != EFI_SUCCESS)
>> > > +             return ret;
>> > > +
>> > > +     ret = EFI_CALL(bs->open_protocol(handle, &efi_guid_device_path,
>> > > +                                      (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)
>
>
> Maybe this comment is not related to this current commit, but on loading http file we do not set maximum size to download. I.e. if the image will be too big it will cause segfault I think.

The patch #1(net: wget: prevent overwriting reserved memory) checks valid memory
during wget transfers on the fly, so at least segfault will not occur.
The same checking with LMB is done in TFTP of current U-Boot code.
Anyway, I guess we may need some rework of this valid memory check for
lwip variant.

>
>>
>> > > +{
>> > > +     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;
>
>
> maybe something like env_get_hex("filesize", 0); here?
> Size is potentially limited with half of int type here.

Yes, env_get_hex("filesize", 0); is proper here. Thank you.

>
>>
>> > > +
>> > > +     /*
>> > > +      * If the file extension is ".iso" or ".img", mount it and try to load
>> > > +      * the default file.
>> > > +      * If the file is 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)) {
>
>
> Is it possible to check magic numbers rather than naming?
> I see that at least .iso has one:
> https://en.wikipedia.org/wiki/List_of_file_signatures

I' not sure how the file signature is common for iso images.
I checked some fedora, suse, debian installer iso images
but these do not have signature at the beginning of files.

>
>>
>> > > +             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 (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
>> > > +             efi_handle_t mem_handle = NULL;
>> > > +             struct efi_device_path *file_path = NULL;
>
>
> no need to set file_path to NULL
OK.

>>
>> > > +
>> > > +             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;
>
> EFI_UNSUPPORTED or EFI_NO_MEDIA looks more suitable here.
I will change it to EFI_UNSUPPORTED.

>
>>
>> > > +     }
>> > > +
>> > > +     return ret;
>> > > +}
>> > > +
>> > >  /**
>> > >   * try_load_entry() - try to load image for boot option
>> > >   *
>> > > @@ -211,6 +391,15 @@ 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);
>> > > +             } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
>> > > +                     if (IS_ENABLED(CONFIG_BLKMAP) &&
>> > > +                         IS_ENABLED(CONFIG_CMD_WGET) &&
>> > > +                         IS_ENABLED(CONFIG_CMD_DNS)) {
>
>
> This looks like too many Kconfig options. Maybe there is a way to make some function empty and let LTO do size optimization.

Sorry, I'm not familiar with LTO, could you give some ideas of modification?

Regards,
Masahisa Kojima


>
>>
>> > > +                             ret = try_load_from_uri_path(
>> > > +                                     (struct efi_device_path_uri *)
>> > > +                                             lo.file_path,
>> > > +                                     lo.label, handle);
>> > > +                     }
>> >
>> > is ret initialized in this function?  Otherwise we need to initialize it if
>> > one of the Kconfig options are disabled
>>
>> Thank you, I will add an 'else' statement to set the error here.
>>
>> Thanks,
>> Masahisa Kojima
>>
>> >
>> > >               } else {
>> > >                       file_path = expand_media_path(lo.file_path);
>> > >                       ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
>> > > --
>> > > 2.34.1
>> > >
>> >
>> > Thanks
>> > /Ilias

Re: [PATCH v4 4/8] efi_loader: support boot from URI device path

[Maxim Uvarov]

On Fri, 29 Sept 2023 at 08:13, Masahisa Kojima <masahisa.kojima@linaro.org>
wrote:

> Hi Maxim,
>
> On Thu, 28 Sept 2023 at 20:35, Maxim Uvarov <maxim.uvarov@linaro.org>
> wrote:
> >
> >
> >
> > On Tue, 26 Sept 2023 at 09:01, Masahisa Kojima <
> masahisa.kojima@linaro.org> wrote:
> >>
> >> Hi Ilias,
> >>
> >> On Mon, 25 Sept 2023 at 21:37, Ilias Apalodimas
> >> <ilias.apalodimas@linaro.org> wrote:
> >> >
> >> > On Fri, Sep 22, 2023 at 04:11:15PM +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 | 189
> +++++++++++++++++++++++++++++++++++
> >> > >  1 file changed, 189 insertions(+)
> >> > >
> >> > > diff --git a/lib/efi_loader/efi_bootmgr.c
> b/lib/efi_loader/efi_bootmgr.c
> >> > > index a40762c74c..605be5041e 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,182 @@ out:
> >> > >       return ret;
> >> > >  }
> >> > >
> >> > > +/**
> >> > > + * 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)
> >> > > +{
> >> >
> >> > Maybe I misunderstood you on the previous series.  Wasn't the plan to
> merge
> >> > the patch that adds boot options automatically when a disk is
> scanned? This
> >>
> >> Adding the boot option automatically when a disk is scanned was sent
> >> separately[1] from this series
> >> since this feature is the matter of the timing of automatic boot
> >> option creation and not directly related
> >> to EFI HTTP Boot.
> >> I also included the fixes of the efi_secboot python test failure.
> >> Sorry for confusing you.
> >>
> >> [1]
> https://patchwork.ozlabs.org/project/uboot/patch/20230920084121.1248590-1-masahisa.kojima@linaro.org/
> >>
> >> > code could only look for a boot option with '1234567' in its load
> options
> >> > instead of rescanning all devices with the
> EFI_SIMPLE_FILE_SYSTEM_PROTOCO
> >> > installed
> >>
> >> Now we can scan and load the default file(EFI/BOOT/BOOTAA64.EFI) on the
> fly with
> >> load_default_file_from_blk_dev() function.
> >> The patch #8 of this series "efi_loader: create BlockIo device boot
> >> option" create the boot option
> >> for the block device excluding the logical partition,
> >>
> >> >
> >> > > +     efi_status_t ret;
> >> > > +     efi_handle_t 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 **)&handle)) {
> >> > > +             log_warning("DM_TAG_EFI not found\n");
> >> > > +             return EFI_INVALID_PARAMETER;
> >> > > +     }
> >> > > +
> >> > > +     ret = efi_search_protocol(handle,
> >> > > +
>  &efi_simple_file_system_protocol_guid, &handler);
> >> > > +     if (ret != EFI_SUCCESS)
> >> > > +             return ret;
> >> > > +
> >> > > +     ret = EFI_CALL(bs->open_protocol(handle,
> &efi_guid_device_path,
> >> > > +                                      (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)
> >
> >
> > Maybe this comment is not related to this current commit, but on loading
> http file we do not set maximum size to download. I.e. if the image will be
> too big it will cause segfault I think.
>
> The patch #1(net: wget: prevent overwriting reserved memory) checks valid
> memory
> during wget transfers on the fly, so at least segfault will not occur.
> The same checking with LMB is done in TFTP of current U-Boot code.
> Anyway, I guess we may need some rework of this valid memory check for
> lwip variant.
>
> >
> >>
> >> > > +{
> >> > > +     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;
> >
> >
> > maybe something like env_get_hex("filesize", 0); here?
> > Size is potentially limited with half of int type here.
>
> Yes, env_get_hex("filesize", 0); is proper here. Thank you.
>
> >
> >>
> >> > > +
> >> > > +     /*
> >> > > +      * If the file extension is ".iso" or ".img", mount it and
> try to load
> >> > > +      * the default file.
> >> > > +      * If the file is 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)) {
> >
> >
> > Is it possible to check magic numbers rather than naming?
> > I see that at least .iso has one:
> > https://en.wikipedia.org/wiki/List_of_file_signatures
>
> I' not sure how the file signature is common for iso images.
> I checked some fedora, suse, debian installer iso images
> but these do not have signature at the beginning of files.
>
> >
> >>
> >> > > +             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 (efi_check_pe((void *)image_addr, image_size, NULL)
> == EFI_SUCCESS) {
> >> > > +             efi_handle_t mem_handle = NULL;
> >> > > +             struct efi_device_path *file_path = NULL;
> >
> >
> > no need to set file_path to NULL
> OK.
>
> >>
> >> > > +
> >> > > +             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;
> >
> > EFI_UNSUPPORTED or EFI_NO_MEDIA looks more suitable here.
> I will change it to EFI_UNSUPPORTED.
>
> >
> >>
> >> > > +     }
> >> > > +
> >> > > +     return ret;
> >> > > +}
> >> > > +
> >> > >  /**
> >> > >   * try_load_entry() - try to load image for boot option
> >> > >   *
> >> > > @@ -211,6 +391,15 @@ 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);
> >> > > +             } else if (EFI_DP_TYPE(lo.file_path,
> MESSAGING_DEVICE, MSG_URI)) {
> >> > > +                     if (IS_ENABLED(CONFIG_BLKMAP) &&
> >> > > +                         IS_ENABLED(CONFIG_CMD_WGET) &&
> >> > > +                         IS_ENABLED(CONFIG_CMD_DNS)) {
> >
> >
> > This looks like too many Kconfig options. Maybe there is a way to make
> some function empty and let LTO do size optimization.
>
> Sorry, I'm not familiar with LTO, could you give some ideas of
> modification?
>
>
LTO is link time optimization. Idea is that if linker understands that
there is no direct reference for some compiled code it will get rid of it
in the final binary.
In my opinion here if EFI is enabled then CONFIG_BLKMAP, CONFIG_CMD_WGET,
CONFIG_CMD_DNS has to be also enabled. Or we will have one more
configuration
variant and more likely there will be no users for that optimization.  It
might be make sense to use IS_ENABLED(CONFIG_NET) to enable and disable all
networking
for the EFI. I think if you disable CONFIG_NET then LTO should optimize the
final binary even if you have all this EFI code, because there will be no
net rx/tx function for that.

BR,
Maxim.

Regards,
> Masahisa Kojima
>
>
> >
> >>
> >> > > +                             ret = try_load_from_uri_path(
> >> > > +                                     (struct efi_device_path_uri *)
> >> > > +                                             lo.file_path,
> >> > > +                                     lo.label, handle);
> >> > > +                     }
> >> >
> >> > is ret initialized in this function?  Otherwise we need to initialize
> it if
> >> > one of the Kconfig options are disabled
> >>
> >> Thank you, I will add an 'else' statement to set the error here.
> >>
> >> Thanks,
> >> Masahisa Kojima
> >>
> >> >
> >> > >               } else {
> >> > >                       file_path = expand_media_path(lo.file_path);
> >> > >                       ret = EFI_CALL(efi_load_image(true, efi_root,
> file_path,
> >> > > --
> >> > > 2.34.1
> >> > >
> >> >
> >> > Thanks
> >> > /Ilias
>

Re: [PATCH v4 4/8] efi_loader: support boot from URI device path

[Masahisa Kojima]

Hi Maxim,

On Fri, 29 Sept 2023 at 15:24, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
>
>
> On Fri, 29 Sept 2023 at 08:13, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>>
>> Hi Maxim,
>>
>> On Thu, 28 Sept 2023 at 20:35, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>> >
>> >
>> >
>> > On Tue, 26 Sept 2023 at 09:01, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>> >>
>> >> Hi Ilias,
>> >>
>> >> On Mon, 25 Sept 2023 at 21:37, Ilias Apalodimas
>> >> <ilias.apalodimas@linaro.org> wrote:
>> >> >
>> >> > On Fri, Sep 22, 2023 at 04:11:15PM +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 | 189 +++++++++++++++++++++++++++++++++++
>> >> > >  1 file changed, 189 insertions(+)
>> >> > >
>> >> > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>> >> > > index a40762c74c..605be5041e 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,182 @@ out:
>> >> > >       return ret;
>> >> > >  }
>> >> > >
>> >> > > +/**
>> >> > > + * 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)
>> >> > > +{
>> >> >
>> >> > Maybe I misunderstood you on the previous series.  Wasn't the plan to merge
>> >> > the patch that adds boot options automatically when a disk is scanned? This
>> >>
>> >> Adding the boot option automatically when a disk is scanned was sent
>> >> separately[1] from this series
>> >> since this feature is the matter of the timing of automatic boot
>> >> option creation and not directly related
>> >> to EFI HTTP Boot.
>> >> I also included the fixes of the efi_secboot python test failure.
>> >> Sorry for confusing you.
>> >>
>> >> [1] https://patchwork.ozlabs.org/project/uboot/patch/20230920084121.1248590-1-masahisa.kojima@linaro.org/
>> >>
>> >> > code could only look for a boot option with '1234567' in its load options
>> >> > instead of rescanning all devices with the EFI_SIMPLE_FILE_SYSTEM_PROTOCO
>> >> > installed
>> >>
>> >> Now we can scan and load the default file(EFI/BOOT/BOOTAA64.EFI) on the fly with
>> >> load_default_file_from_blk_dev() function.
>> >> The patch #8 of this series "efi_loader: create BlockIo device boot
>> >> option" create the boot option
>> >> for the block device excluding the logical partition,
>> >>
>> >> >
>> >> > > +     efi_status_t ret;
>> >> > > +     efi_handle_t 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 **)&handle)) {
>> >> > > +             log_warning("DM_TAG_EFI not found\n");
>> >> > > +             return EFI_INVALID_PARAMETER;
>> >> > > +     }
>> >> > > +
>> >> > > +     ret = efi_search_protocol(handle,
>> >> > > +                               &efi_simple_file_system_protocol_guid, &handler);
>> >> > > +     if (ret != EFI_SUCCESS)
>> >> > > +             return ret;
>> >> > > +
>> >> > > +     ret = EFI_CALL(bs->open_protocol(handle, &efi_guid_device_path,
>> >> > > +                                      (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)
>> >
>> >
>> > Maybe this comment is not related to this current commit, but on loading http file we do not set maximum size to download. I.e. if the image will be too big it will cause segfault I think.
>>
>> The patch #1(net: wget: prevent overwriting reserved memory) checks valid memory
>> during wget transfers on the fly, so at least segfault will not occur.
>> The same checking with LMB is done in TFTP of current U-Boot code.
>> Anyway, I guess we may need some rework of this valid memory check for
>> lwip variant.
>>
>> >
>> >>
>> >> > > +{
>> >> > > +     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;
>> >
>> >
>> > maybe something like env_get_hex("filesize", 0); here?
>> > Size is potentially limited with half of int type here.
>>
>> Yes, env_get_hex("filesize", 0); is proper here. Thank you.
>>
>> >
>> >>
>> >> > > +
>> >> > > +     /*
>> >> > > +      * If the file extension is ".iso" or ".img", mount it and try to load
>> >> > > +      * the default file.
>> >> > > +      * If the file is 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)) {
>> >
>> >
>> > Is it possible to check magic numbers rather than naming?
>> > I see that at least .iso has one:
>> > https://en.wikipedia.org/wiki/List_of_file_signatures
>>
>> I' not sure how the file signature is common for iso images.
>> I checked some fedora, suse, debian installer iso images
>> but these do not have signature at the beginning of files.
>>
>> >
>> >>
>> >> > > +             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 (efi_check_pe((void *)image_addr, image_size, NULL) == EFI_SUCCESS) {
>> >> > > +             efi_handle_t mem_handle = NULL;
>> >> > > +             struct efi_device_path *file_path = NULL;
>> >
>> >
>> > no need to set file_path to NULL
>> OK.
>>
>> >>
>> >> > > +
>> >> > > +             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;
>> >
>> > EFI_UNSUPPORTED or EFI_NO_MEDIA looks more suitable here.
>> I will change it to EFI_UNSUPPORTED.
>>
>> >
>> >>
>> >> > > +     }
>> >> > > +
>> >> > > +     return ret;
>> >> > > +}
>> >> > > +
>> >> > >  /**
>> >> > >   * try_load_entry() - try to load image for boot option
>> >> > >   *
>> >> > > @@ -211,6 +391,15 @@ 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);
>> >> > > +             } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
>> >> > > +                     if (IS_ENABLED(CONFIG_BLKMAP) &&
>> >> > > +                         IS_ENABLED(CONFIG_CMD_WGET) &&
>> >> > > +                         IS_ENABLED(CONFIG_CMD_DNS)) {
>> >
>> >
>> > This looks like too many Kconfig options. Maybe there is a way to make some function empty and let LTO do size optimization.
>>
>> Sorry, I'm not familiar with LTO, could you give some ideas of modification?
>>
>
> LTO is link time optimization. Idea is that if linker understands that there is no direct reference for some compiled code it will get rid of it in the final binary.
> In my opinion here if EFI is enabled then CONFIG_BLKMAP, CONFIG_CMD_WGET, CONFIG_CMD_DNS has to be also enabled. Or we will have one more configuration
> variant and more likely there will be no users for that optimization.  It might be make sense to use IS_ENABLED(CONFIG_NET) to enable and disable all networking
> for the EFI. I think if you disable CONFIG_NET then LTO should optimize the final binary even if you have all this EFI code, because there will be no net rx/tx function for that.

Probably everyone has a different opinion here, I would like to narrow
down to the EFI HTTP Boot.
I will create a new Kconfig option CONFIG_EFI_HTTP_BOOT,
it depends on CONFIG_BLKMAP, CONFIG_CMD_WGET, CONFIG_CMD_DNS.
efibootmgr only checks if CONFIG_EFI_HTTP_BOOT is enabled.

Thanks,
Masahisa Kojima

>
> BR,
> Maxim.
>
>> Regards,
>> Masahisa Kojima
>>
>>
>> >
>> >>
>> >> > > +                             ret = try_load_from_uri_path(
>> >> > > +                                     (struct efi_device_path_uri *)
>> >> > > +                                             lo.file_path,
>> >> > > +                                     lo.label, handle);
>> >> > > +                     }
>> >> >
>> >> > is ret initialized in this function?  Otherwise we need to initialize it if
>> >> > one of the Kconfig options are disabled
>> >>
>> >> Thank you, I will add an 'else' statement to set the error here.
>> >>
>> >> Thanks,
>> >> Masahisa Kojima
>> >>
>> >> >
>> >> > >               } else {
>> >> > >                       file_path = expand_media_path(lo.file_path);
>> >> > >                       ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
>> >> > > --
>> >> > > 2.34.1
>> >> > >
>> >> >
>> >> > Thanks
>> >> > /Ilias

Re: [PATCH v4 4/8] efi_loader: support boot from URI device path

[Maxim Uvarov]

On Fri, 29 Sept 2023 at 13:00, Masahisa Kojima <masahisa.kojima@linaro.org>
wrote:

> Hi Maxim,
>
> On Fri, 29 Sept 2023 at 15:24, Maxim Uvarov <maxim.uvarov@linaro.org>
> wrote:
> >
> >
> >
> > On Fri, 29 Sept 2023 at 08:13, Masahisa Kojima <
> masahisa.kojima@linaro.org> wrote:
> >>
> >> Hi Maxim,
> >>
> >> On Thu, 28 Sept 2023 at 20:35, Maxim Uvarov <maxim.uvarov@linaro.org>
> wrote:
> >> >
> >> >
> >> >
> >> > On Tue, 26 Sept 2023 at 09:01, Masahisa Kojima <
> masahisa.kojima@linaro.org> wrote:
> >> >>
> >> >> Hi Ilias,
> >> >>
> >> >> On Mon, 25 Sept 2023 at 21:37, Ilias Apalodimas
> >> >> <ilias.apalodimas@linaro.org> wrote:
> >> >> >
> >> >> > On Fri, Sep 22, 2023 at 04:11:15PM +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 | 189
> +++++++++++++++++++++++++++++++++++
> >> >> > >  1 file changed, 189 insertions(+)
> >> >> > >
> >> >> > > diff --git a/lib/efi_loader/efi_bootmgr.c
> b/lib/efi_loader/efi_bootmgr.c
> >> >> > > index a40762c74c..605be5041e 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,182 @@ out:
> >> >> > >       return ret;
> >> >> > >  }
> >> >> > >
> >> >> > > +/**
> >> >> > > + * 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)
> >> >> > > +{
> >> >> >
> >> >> > Maybe I misunderstood you on the previous series.  Wasn't the plan
> to merge
> >> >> > the patch that adds boot options automatically when a disk is
> scanned? This
> >> >>
> >> >> Adding the boot option automatically when a disk is scanned was sent
> >> >> separately[1] from this series
> >> >> since this feature is the matter of the timing of automatic boot
> >> >> option creation and not directly related
> >> >> to EFI HTTP Boot.
> >> >> I also included the fixes of the efi_secboot python test failure.
> >> >> Sorry for confusing you.
> >> >>
> >> >> [1]
> https://patchwork.ozlabs.org/project/uboot/patch/20230920084121.1248590-1-masahisa.kojima@linaro.org/
> >> >>
> >> >> > code could only look for a boot option with '1234567' in its load
> options
> >> >> > instead of rescanning all devices with the
> EFI_SIMPLE_FILE_SYSTEM_PROTOCO
> >> >> > installed
> >> >>
> >> >> Now we can scan and load the default file(EFI/BOOT/BOOTAA64.EFI) on
> the fly with
> >> >> load_default_file_from_blk_dev() function.
> >> >> The patch #8 of this series "efi_loader: create BlockIo device boot
> >> >> option" create the boot option
> >> >> for the block device excluding the logical partition,
> >> >>
> >> >> >
> >> >> > > +     efi_status_t ret;
> >> >> > > +     efi_handle_t 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 **)&handle)) {
> >> >> > > +             log_warning("DM_TAG_EFI not found\n");
> >> >> > > +             return EFI_INVALID_PARAMETER;
> >> >> > > +     }
> >> >> > > +
> >> >> > > +     ret = efi_search_protocol(handle,
> >> >> > > +
>  &efi_simple_file_system_protocol_guid, &handler);
> >> >> > > +     if (ret != EFI_SUCCESS)
> >> >> > > +             return ret;
> >> >> > > +
> >> >> > > +     ret = EFI_CALL(bs->open_protocol(handle,
> &efi_guid_device_path,
> >> >> > > +                                      (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)
> >> >
> >> >
> >> > Maybe this comment is not related to this current commit, but on
> loading http file we do not set maximum size to download. I.e. if the image
> will be too big it will cause segfault I think.
> >>
> >> The patch #1(net: wget: prevent overwriting reserved memory) checks
> valid memory
> >> during wget transfers on the fly, so at least segfault will not occur.
> >> The same checking with LMB is done in TFTP of current U-Boot code.
> >> Anyway, I guess we may need some rework of this valid memory check for
> >> lwip variant.
> >>
> >> >
> >> >>
> >> >> > > +{
> >> >> > > +     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;
> >> >
> >> >
> >> > maybe something like env_get_hex("filesize", 0); here?
> >> > Size is potentially limited with half of int type here.
> >>
> >> Yes, env_get_hex("filesize", 0); is proper here. Thank you.
> >>
> >> >
> >> >>
> >> >> > > +
> >> >> > > +     /*
> >> >> > > +      * If the file extension is ".iso" or ".img", mount it and
> try to load
> >> >> > > +      * the default file.
> >> >> > > +      * If the file is 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)) {
> >> >
> >> >
> >> > Is it possible to check magic numbers rather than naming?
> >> > I see that at least .iso has one:
> >> > https://en.wikipedia.org/wiki/List_of_file_signatures
> >>
> >> I' not sure how the file signature is common for iso images.
> >> I checked some fedora, suse, debian installer iso images
> >> but these do not have signature at the beginning of files.
> >>
> >> >
> >> >>
> >> >> > > +             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 (efi_check_pe((void *)image_addr, image_size,
> NULL) == EFI_SUCCESS) {
> >> >> > > +             efi_handle_t mem_handle = NULL;
> >> >> > > +             struct efi_device_path *file_path = NULL;
> >> >
> >> >
> >> > no need to set file_path to NULL
> >> OK.
> >>
> >> >>
> >> >> > > +
> >> >> > > +             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;
> >> >
> >> > EFI_UNSUPPORTED or EFI_NO_MEDIA looks more suitable here.
> >> I will change it to EFI_UNSUPPORTED.
> >>
> >> >
> >> >>
> >> >> > > +     }
> >> >> > > +
> >> >> > > +     return ret;
> >> >> > > +}
> >> >> > > +
> >> >> > >  /**
> >> >> > >   * try_load_entry() - try to load image for boot option
> >> >> > >   *
> >> >> > > @@ -211,6 +391,15 @@ 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);
> >> >> > > +             } else if (EFI_DP_TYPE(lo.file_path,
> MESSAGING_DEVICE, MSG_URI)) {
> >> >> > > +                     if (IS_ENABLED(CONFIG_BLKMAP) &&
> >> >> > > +                         IS_ENABLED(CONFIG_CMD_WGET) &&
> >> >> > > +                         IS_ENABLED(CONFIG_CMD_DNS)) {
> >> >
> >> >
> >> > This looks like too many Kconfig options. Maybe there is a way to
> make some function empty and let LTO do size optimization.
> >>
> >> Sorry, I'm not familiar with LTO, could you give some ideas of
> modification?
> >>
> >
> > LTO is link time optimization. Idea is that if linker understands that
> there is no direct reference for some compiled code it will get rid of it
> in the final binary.
> > In my opinion here if EFI is enabled then CONFIG_BLKMAP,
> CONFIG_CMD_WGET, CONFIG_CMD_DNS has to be also enabled. Or we will have one
> more configuration
> > variant and more likely there will be no users for that optimization.
> It might be make sense to use IS_ENABLED(CONFIG_NET) to enable and disable
> all networking
> > for the EFI. I think if you disable CONFIG_NET then LTO should optimize
> the final binary even if you have all this EFI code, because there will be
> no net rx/tx function for that.
>
> Probably everyone has a different opinion here, I would like to narrow
> down to the EFI HTTP Boot.
> I will create a new Kconfig option CONFIG_EFI_HTTP_BOOT,
> it depends on CONFIG_BLKMAP, CONFIG_CMD_WGET, CONFIG_CMD_DNS.
> efibootmgr only checks if CONFIG_EFI_HTTP_BOOT is enabled.
>
>
I think that will work for now, letter I think CONFIG_CMD_X also can go
away for this case. I mean depend on cmd commands, then functionality.


> Thanks,
> Masahisa Kojima
>
> >
> > BR,
> > Maxim.
> >
> >> Regards,
> >> Masahisa Kojima
> >>
> >>
> >> >
> >> >>
> >> >> > > +                             ret = try_load_from_uri_path(
> >> >> > > +                                     (struct
> efi_device_path_uri *)
> >> >> > > +                                             lo.file_path,
> >> >> > > +                                     lo.label, handle);
> >> >> > > +                     }
> >> >> >
> >> >> > is ret initialized in this function?  Otherwise we need to
> initialize it if
> >> >> > one of the Kconfig options are disabled
> >> >>
> >> >> Thank you, I will add an 'else' statement to set the error here.
> >> >>
> >> >> Thanks,
> >> >> Masahisa Kojima
> >> >>
> >> >> >
> >> >> > >               } else {
> >> >> > >                       file_path =
> expand_media_path(lo.file_path);
> >> >> > >                       ret = EFI_CALL(efi_load_image(true,
> efi_root, file_path,
> >> >> > > --
> >> >> > > 2.34.1
> >> >> > >
> >> >> >
> >> >> > Thanks
> >> >> > /Ilias
>