* [PATCH v7 1/9] net: wget: prevent overwriting reserved memory
2023-10-16 6:45 [PATCH v7 0/9] Add EFI HTTP boot support Masahisa Kojima
@ 2023-10-16 6:45 ` Masahisa Kojima
2023-10-16 6:45 ` [PATCH v7 2/9] net: wget: add wget with dns utility function Masahisa Kojima
` (7 subsequent siblings)
8 siblings, 0 replies; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-16 6:45 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, Masahisa Kojima, Ramon Fried,
Joe Hershberger
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>
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
---
net/wget.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 73 insertions(+), 7 deletions(-)
diff --git a/net/wget.c b/net/wget.c
index 8bb4d72db1..6f97eb1d12 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";
@@ -56,6 +60,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
@@ -64,10 +91,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);
@@ -248,25 +290,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;
+ }
}
}
}
@@ -338,6 +394,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;
}
@@ -428,6 +485,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
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v7 2/9] net: wget: add wget with dns utility function
2023-10-16 6:45 [PATCH v7 0/9] Add EFI HTTP boot support Masahisa Kojima
2023-10-16 6:45 ` [PATCH v7 1/9] net: wget: prevent overwriting reserved memory Masahisa Kojima
@ 2023-10-16 6:45 ` Masahisa Kojima
2023-10-16 6:45 ` [PATCH v7 3/9] blk: blkmap: add ramdisk creation " Masahisa Kojima
` (6 subsequent siblings)
8 siblings, 0 replies; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-16 6:45 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, Masahisa Kojima, Ramon Fried,
Joe Hershberger
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>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
---
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 6f97eb1d12..2087146b37 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;
@@ -512,3 +513,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
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v7 3/9] blk: blkmap: add ramdisk creation utility function
2023-10-16 6:45 [PATCH v7 0/9] Add EFI HTTP boot support Masahisa Kojima
2023-10-16 6:45 ` [PATCH v7 1/9] net: wget: prevent overwriting reserved memory Masahisa Kojima
2023-10-16 6:45 ` [PATCH v7 2/9] net: wget: add wget with dns utility function Masahisa Kojima
@ 2023-10-16 6:45 ` Masahisa Kojima
2023-10-16 6:45 ` [PATCH v7 4/9] efi_loader: create default file boot option Masahisa Kojima
` (5 subsequent siblings)
8 siblings, 0 replies; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-16 6:45 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, Masahisa Kojima, Tobias Waldekranz,
Jaehoon Chung
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>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
drivers/block/Makefile | 3 +-
drivers/block/blkmap.c | 15 ----------
drivers/block/blkmap_helper.c | 53 +++++++++++++++++++++++++++++++++++
include/blkmap.h | 29 +++++++++++++++++++
4 files changed, 84 insertions(+), 16 deletions(-)
create mode 100644 drivers/block/blkmap_helper.c
diff --git a/drivers/block/Makefile b/drivers/block/Makefile
index a161d145fd..ec0575d135 100644
--- a/drivers/block/Makefile
+++ b/drivers/block/Makefile
@@ -14,7 +14,8 @@ obj-$(CONFIG_IDE) += ide.o
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_$(SPL_TPL_)BLKMAP) += blkmap.o
+obj-$(CONFIG_$(SPL_TPL_)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 149a4cac3e..21201409ed 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
^ permalink raw reply related [flat|nested] 33+ messages in thread* [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-16 6:45 [PATCH v7 0/9] Add EFI HTTP boot support Masahisa Kojima
` (2 preceding siblings ...)
2023-10-16 6:45 ` [PATCH v7 3/9] blk: blkmap: add ramdisk creation " Masahisa Kojima
@ 2023-10-16 6:45 ` Masahisa Kojima
2023-10-16 7:06 ` Heinrich Schuchardt
2023-10-16 6:45 ` [PATCH v7 5/9] efi_loader: support boot from URI device path Masahisa Kojima
` (4 subsequent siblings)
8 siblings, 1 reply; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-16 6:45 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, Masahisa Kojima
Current efibootmgr automatically creates the
boot options of all disks and partitions installing
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
Some of the automatically created boot options are
useless if the disk and partition does not have
the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
This commit only creates the boot option if the disk and
partition have the default file so that system can directly
boot from it.
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
lib/efi_loader/efi_bootmgr.c | 86 ++++++++++++++++++++++++++----------
1 file changed, 63 insertions(+), 23 deletions(-)
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index a40762c74c..c8cf1c5506 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -355,40 +355,70 @@ 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;
struct efi_load_option lo;
char buf[BOOTMENU_DEVICE_NAME_MAX];
- struct efi_device_path *device_path;
+ struct efi_device_path *device_path, *full_path, *dp, *fp;
struct efi_device_path *short_dp;
+ struct efi_file_handle *root, *f;
+ struct efi_simple_file_system_protocol *file_system;
+ u16 *default_file_path = NULL;
- ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
+ ret = efi_search_protocol(volume_handles[i],
+ &efi_guid_device_path, &handler);
if (ret != EFI_SUCCESS)
continue;
- ret = efi_protocol_open(handler, (void **)&device_path,
- efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+
+ device_path = handler->protocol_interface;
+ full_path = efi_dp_from_file(device_path,
+ "/EFI/BOOT/" BOOTEFI_NAME);
+
+ /* check whether the partition or disk have the default file */
+ ret = efi_dp_split_file_path(full_path, &dp, &fp);
+ if (ret != EFI_SUCCESS || !fp)
+ goto next_entry;
+
+ default_file_path = efi_dp_str(fp);
+ if (!default_file_path)
+ goto next_entry;
+
+ ret = efi_search_protocol(volume_handles[i],
+ &efi_simple_file_system_protocol_guid,
+ &handler);
if (ret != EFI_SUCCESS)
- continue;
+ goto next_entry;
+
+ file_system = handler->protocol_interface;
+ ret = EFI_CALL(file_system->open_volume(file_system, &root));
+ if (ret != EFI_SUCCESS)
+ goto next_entry;
+
+ ret = EFI_CALL(root->open(root, &f, default_file_path,
+ EFI_FILE_MODE_READ, 0));
+ if (ret != EFI_SUCCESS)
+ goto next_entry;
+
+ EFI_CALL(f->close(f));
ret = efi_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
if (ret != EFI_SUCCESS)
- continue;
+ goto next_entry;
p = dev_name;
utf8_utf16_strncpy(&p, buf, strlen(buf));
/* prefer to short form device path */
- short_dp = efi_dp_shorten(device_path);
- if (short_dp)
- device_path = short_dp;
+ short_dp = efi_dp_shorten(full_path);
+ device_path = short_dp ? short_dp : full_path;
lo.label = dev_name;
lo.attributes = LOAD_OPTION_ACTIVE;
@@ -396,24 +426,35 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
/*
* Set the dedicated guid to optional_data, it is used to identify
- * the boot option that automatically generated by the bootmenu.
+ * the boot option that automatically generated by the efibootmgr.
* efi_serialize_load_option() expects optional_data is null-terminated
* utf8 string, so set the "1234567" string to allocate enough space
* 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) {
- ret = EFI_OUT_OF_RESOURCES;
- goto out;
+ opt[num].size = efi_serialize_load_option(&lo, (u8 **)&opt[num].lo);
+ if (!opt[num].size) {
+ efi_free_pool(full_path);
+ efi_free_pool(dp);
+ efi_free_pool(fp);
+ efi_free_pool(default_file_path);
+ return EFI_OUT_OF_RESOURCES;
}
/* 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++;
+
+next_entry:
+ efi_free_pool(full_path);
+ efi_free_pool(dp);
+ efi_free_pool(fp);
+ efi_free_pool(default_file_path);
}
-out:
- return ret;
+ *count = num;
+
+ return EFI_SUCCESS;
}
/**
@@ -642,7 +683,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.
+ * and create the boot option with default file if the file exists.
* This function also provide the BOOT#### variable maintenance for
* the media device entries.
* - Automatically create the BOOT#### variable for the newly detected device,
@@ -674,8 +715,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;
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-16 6:45 ` [PATCH v7 4/9] efi_loader: create default file boot option Masahisa Kojima
@ 2023-10-16 7:06 ` Heinrich Schuchardt
2023-10-16 12:31 ` Ilias Apalodimas
0 siblings, 1 reply; 33+ messages in thread
From: Heinrich Schuchardt @ 2023-10-16 7:06 UTC (permalink / raw)
To: Masahisa Kojima, u-boot
Cc: Ilias Apalodimas, Simon Glass, Takahiro Akashi, Michal Simek
Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
>Current efibootmgr automatically creates the
>boot options of all disks and partitions installing
>EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
>Some of the automatically created boot options are
>useless if the disk and partition does not have
>the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
>
>This commit only creates the boot option if the disk and
>partition have the default file so that system can directly
>boot from it.
I don't directly see the user benefit.
Reading all file systems will increase the boot time. Shouldn't we avoid this?
What does EDK II do?
Does the UEFI specification warrant this change?
Best regards
Heinrich
>
>Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>---
> lib/efi_loader/efi_bootmgr.c | 86 ++++++++++++++++++++++++++----------
> 1 file changed, 63 insertions(+), 23 deletions(-)
>
>diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>index a40762c74c..c8cf1c5506 100644
>--- a/lib/efi_loader/efi_bootmgr.c
>+++ b/lib/efi_loader/efi_bootmgr.c
>@@ -355,40 +355,70 @@ 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;
> struct efi_load_option lo;
> char buf[BOOTMENU_DEVICE_NAME_MAX];
>- struct efi_device_path *device_path;
>+ struct efi_device_path *device_path, *full_path, *dp, *fp;
> struct efi_device_path *short_dp;
>+ struct efi_file_handle *root, *f;
>+ struct efi_simple_file_system_protocol *file_system;
>+ u16 *default_file_path = NULL;
>
>- ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
>+ ret = efi_search_protocol(volume_handles[i],
>+ &efi_guid_device_path, &handler);
> if (ret != EFI_SUCCESS)
> continue;
>- ret = efi_protocol_open(handler, (void **)&device_path,
>- efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>+
>+ device_path = handler->protocol_interface;
>+ full_path = efi_dp_from_file(device_path,
>+ "/EFI/BOOT/" BOOTEFI_NAME);
>+
>+ /* check whether the partition or disk have the default file */
>+ ret = efi_dp_split_file_path(full_path, &dp, &fp);
>+ if (ret != EFI_SUCCESS || !fp)
>+ goto next_entry;
>+
>+ default_file_path = efi_dp_str(fp);
>+ if (!default_file_path)
>+ goto next_entry;
>+
>+ ret = efi_search_protocol(volume_handles[i],
>+ &efi_simple_file_system_protocol_guid,
>+ &handler);
> if (ret != EFI_SUCCESS)
>- continue;
>+ goto next_entry;
>+
>+ file_system = handler->protocol_interface;
>+ ret = EFI_CALL(file_system->open_volume(file_system, &root));
>+ if (ret != EFI_SUCCESS)
>+ goto next_entry;
>+
>+ ret = EFI_CALL(root->open(root, &f, default_file_path,
>+ EFI_FILE_MODE_READ, 0));
>+ if (ret != EFI_SUCCESS)
>+ goto next_entry;
>+
>+ EFI_CALL(f->close(f));
>
> ret = efi_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
> if (ret != EFI_SUCCESS)
>- continue;
>+ goto next_entry;
>
> p = dev_name;
> utf8_utf16_strncpy(&p, buf, strlen(buf));
>
> /* prefer to short form device path */
>- short_dp = efi_dp_shorten(device_path);
>- if (short_dp)
>- device_path = short_dp;
>+ short_dp = efi_dp_shorten(full_path);
>+ device_path = short_dp ? short_dp : full_path;
>
> lo.label = dev_name;
> lo.attributes = LOAD_OPTION_ACTIVE;
>@@ -396,24 +426,35 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
> lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
> /*
> * Set the dedicated guid to optional_data, it is used to identify
>- * the boot option that automatically generated by the bootmenu.
>+ * the boot option that automatically generated by the efibootmgr.
> * efi_serialize_load_option() expects optional_data is null-terminated
> * utf8 string, so set the "1234567" string to allocate enough space
> * 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) {
>- ret = EFI_OUT_OF_RESOURCES;
>- goto out;
>+ opt[num].size = efi_serialize_load_option(&lo, (u8 **)&opt[num].lo);
>+ if (!opt[num].size) {
>+ efi_free_pool(full_path);
>+ efi_free_pool(dp);
>+ efi_free_pool(fp);
>+ efi_free_pool(default_file_path);
>+ return EFI_OUT_OF_RESOURCES;
> }
> /* 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++;
>+
>+next_entry:
>+ efi_free_pool(full_path);
>+ efi_free_pool(dp);
>+ efi_free_pool(fp);
>+ efi_free_pool(default_file_path);
> }
>
>-out:
>- return ret;
>+ *count = num;
>+
>+ return EFI_SUCCESS;
> }
>
> /**
>@@ -642,7 +683,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.
>+ * and create the boot option with default file if the file exists.
> * This function also provide the BOOT#### variable maintenance for
> * the media device entries.
> * - Automatically create the BOOT#### variable for the newly detected device,
>@@ -674,8 +715,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;
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-16 7:06 ` Heinrich Schuchardt
@ 2023-10-16 12:31 ` Ilias Apalodimas
2023-10-16 12:46 ` Heinrich Schuchardt
0 siblings, 1 reply; 33+ messages in thread
From: Ilias Apalodimas @ 2023-10-16 12:31 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Masahisa Kojima, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
Hi Heinrich,
On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
> >Current efibootmgr automatically creates the
> >boot options of all disks and partitions installing
> >EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> >Some of the automatically created boot options are
> >useless if the disk and partition does not have
> >the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> >
> >This commit only creates the boot option if the disk and
> >partition have the default file so that system can directly
> >boot from it.
>
> I don't directly see the user benefit.
The user can add an HTTP boot option now and the installer will
automatically start. That would allow products to ship with a single
boot option provisioned and run an installer on first boot
>
> Reading all file systems will increase the boot time. Shouldn't we avoid this?
Any idea what would be an alternative? But when we added the
automatic boot options we said we should avoid dynamic scanning and
store results in a file. This is doing a similar thing. The only
difference is that it mounts the iso image before adding the boot
option.
>
> What does EDK II do?
No Idea :)
Thanks
/Ilias
>
> Does the UEFI specification warrant this change?
>
> Best regards
>
> Heinrich
>
> >
> >Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >---
> > lib/efi_loader/efi_bootmgr.c | 86 ++++++++++++++++++++++++++----------
> > 1 file changed, 63 insertions(+), 23 deletions(-)
> >
> >diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >index a40762c74c..c8cf1c5506 100644
> >--- a/lib/efi_loader/efi_bootmgr.c
> >+++ b/lib/efi_loader/efi_bootmgr.c
> >@@ -355,40 +355,70 @@ 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;
> > struct efi_load_option lo;
> > char buf[BOOTMENU_DEVICE_NAME_MAX];
> >- struct efi_device_path *device_path;
> >+ struct efi_device_path *device_path, *full_path, *dp, *fp;
> > struct efi_device_path *short_dp;
> >+ struct efi_file_handle *root, *f;
> >+ struct efi_simple_file_system_protocol *file_system;
> >+ u16 *default_file_path = NULL;
> >
> >- ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
> >+ ret = efi_search_protocol(volume_handles[i],
> >+ &efi_guid_device_path, &handler);
> > if (ret != EFI_SUCCESS)
> > continue;
> >- ret = efi_protocol_open(handler, (void **)&device_path,
> >- efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >+
> >+ device_path = handler->protocol_interface;
> >+ full_path = efi_dp_from_file(device_path,
> >+ "/EFI/BOOT/" BOOTEFI_NAME);
> >+
> >+ /* check whether the partition or disk have the default file */
> >+ ret = efi_dp_split_file_path(full_path, &dp, &fp);
> >+ if (ret != EFI_SUCCESS || !fp)
> >+ goto next_entry;
> >+
> >+ default_file_path = efi_dp_str(fp);
> >+ if (!default_file_path)
> >+ goto next_entry;
> >+
> >+ ret = efi_search_protocol(volume_handles[i],
> >+ &efi_simple_file_system_protocol_guid,
> >+ &handler);
> > if (ret != EFI_SUCCESS)
> >- continue;
> >+ goto next_entry;
> >+
> >+ file_system = handler->protocol_interface;
> >+ ret = EFI_CALL(file_system->open_volume(file_system, &root));
> >+ if (ret != EFI_SUCCESS)
> >+ goto next_entry;
> >+
> >+ ret = EFI_CALL(root->open(root, &f, default_file_path,
> >+ EFI_FILE_MODE_READ, 0));
> >+ if (ret != EFI_SUCCESS)
> >+ goto next_entry;
> >+
> >+ EFI_CALL(f->close(f));
> >
> > ret = efi_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
> > if (ret != EFI_SUCCESS)
> >- continue;
> >+ goto next_entry;
> >
> > p = dev_name;
> > utf8_utf16_strncpy(&p, buf, strlen(buf));
> >
> > /* prefer to short form device path */
> >- short_dp = efi_dp_shorten(device_path);
> >- if (short_dp)
> >- device_path = short_dp;
> >+ short_dp = efi_dp_shorten(full_path);
> >+ device_path = short_dp ? short_dp : full_path;
> >
> > lo.label = dev_name;
> > lo.attributes = LOAD_OPTION_ACTIVE;
> >@@ -396,24 +426,35 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
> > lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
> > /*
> > * Set the dedicated guid to optional_data, it is used to identify
> >- * the boot option that automatically generated by the bootmenu.
> >+ * the boot option that automatically generated by the efibootmgr.
> > * efi_serialize_load_option() expects optional_data is null-terminated
> > * utf8 string, so set the "1234567" string to allocate enough space
> > * 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) {
> >- ret = EFI_OUT_OF_RESOURCES;
> >- goto out;
> >+ opt[num].size = efi_serialize_load_option(&lo, (u8 **)&opt[num].lo);
> >+ if (!opt[num].size) {
> >+ efi_free_pool(full_path);
> >+ efi_free_pool(dp);
> >+ efi_free_pool(fp);
> >+ efi_free_pool(default_file_path);
> >+ return EFI_OUT_OF_RESOURCES;
> > }
> > /* 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++;
> >+
> >+next_entry:
> >+ efi_free_pool(full_path);
> >+ efi_free_pool(dp);
> >+ efi_free_pool(fp);
> >+ efi_free_pool(default_file_path);
> > }
> >
> >-out:
> >- return ret;
> >+ *count = num;
> >+
> >+ return EFI_SUCCESS;
> > }
> >
> > /**
> >@@ -642,7 +683,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.
> >+ * and create the boot option with default file if the file exists.
> > * This function also provide the BOOT#### variable maintenance for
> > * the media device entries.
> > * - Automatically create the BOOT#### variable for the newly detected device,
> >@@ -674,8 +715,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;
> >
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-16 12:31 ` Ilias Apalodimas
@ 2023-10-16 12:46 ` Heinrich Schuchardt
2023-10-16 12:57 ` Ilias Apalodimas
2023-10-16 13:00 ` Masahisa Kojima
0 siblings, 2 replies; 33+ messages in thread
From: Heinrich Schuchardt @ 2023-10-16 12:46 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Masahisa Kojima, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
On 16.10.23 14:31, Ilias Apalodimas wrote:
> Hi Heinrich,
>
> On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>>
>>
>> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
>>> Current efibootmgr automatically creates the
>>> boot options of all disks and partitions installing
>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
>>> Some of the automatically created boot options are
>>> useless if the disk and partition does not have
>>> the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
>>>
>>> This commit only creates the boot option if the disk and
>>> partition have the default file so that system can directly
>>> boot from it.
>>
>> I don't directly see the user benefit.
>
> The user can add an HTTP boot option now and the installer will
> automatically start. That would allow products to ship with a single
> boot option provisioned and run an installer on first boot
This commit is not about HTTP. It changes how boot options for block
devices are created.
>
>>
>> Reading all file systems will increase the boot time. Shouldn't we avoid this?
>
> Any idea what would be an alternative? But when we added the
> automatic boot options we said we should avoid dynamic scanning and
> store results in a file. This is doing a similar thing. The only
> difference is that it mounts the iso image before adding the boot
> option.
The alternative is to keep showing boot options for block devices even
if there is no BOOTxxxx.EFI file.
>
>>
>> What does EDK II do?
>
> No Idea :)
On my workstation I get generated boot options
Boot0001* UEFI:CD/DVD Drive BBS(129,,0x0)
Boot0003* UEFI:Removable Device BBS(130,,0x0)
Boot0004* UEFI:Network Device BBS(131,,0x0)
without any media inserted and without any PXE server available.
Best regards
Heinrich
>
> Thanks
> /Ilias
>>
>> Does the UEFI specification warrant this change?
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>> ---
>>> lib/efi_loader/efi_bootmgr.c | 86 ++++++++++++++++++++++++++----------
>>> 1 file changed, 63 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index a40762c74c..c8cf1c5506 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -355,40 +355,70 @@ 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;
>>> struct efi_load_option lo;
>>> char buf[BOOTMENU_DEVICE_NAME_MAX];
>>> - struct efi_device_path *device_path;
>>> + struct efi_device_path *device_path, *full_path, *dp, *fp;
>>> struct efi_device_path *short_dp;
>>> + struct efi_file_handle *root, *f;
>>> + struct efi_simple_file_system_protocol *file_system;
>>> + u16 *default_file_path = NULL;
>>>
>>> - ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
>>> + ret = efi_search_protocol(volume_handles[i],
>>> + &efi_guid_device_path, &handler);
>>> if (ret != EFI_SUCCESS)
>>> continue;
>>> - ret = efi_protocol_open(handler, (void **)&device_path,
>>> - efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>> +
>>> + device_path = handler->protocol_interface;
>>> + full_path = efi_dp_from_file(device_path,
>>> + "/EFI/BOOT/" BOOTEFI_NAME);
>>> +
>>> + /* check whether the partition or disk have the default file */
>>> + ret = efi_dp_split_file_path(full_path, &dp, &fp);
>>> + if (ret != EFI_SUCCESS || !fp)
>>> + goto next_entry;
>>> +
>>> + default_file_path = efi_dp_str(fp);
>>> + if (!default_file_path)
>>> + goto next_entry;
>>> +
>>> + ret = efi_search_protocol(volume_handles[i],
>>> + &efi_simple_file_system_protocol_guid,
>>> + &handler);
>>> if (ret != EFI_SUCCESS)
>>> - continue;
>>> + goto next_entry;
>>> +
>>> + file_system = handler->protocol_interface;
>>> + ret = EFI_CALL(file_system->open_volume(file_system, &root));
>>> + if (ret != EFI_SUCCESS)
>>> + goto next_entry;
>>> +
>>> + ret = EFI_CALL(root->open(root, &f, default_file_path,
>>> + EFI_FILE_MODE_READ, 0));
>>> + if (ret != EFI_SUCCESS)
>>> + goto next_entry;
>>> +
>>> + EFI_CALL(f->close(f));
>>>
>>> ret = efi_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
>>> if (ret != EFI_SUCCESS)
>>> - continue;
>>> + goto next_entry;
>>>
>>> p = dev_name;
>>> utf8_utf16_strncpy(&p, buf, strlen(buf));
>>>
>>> /* prefer to short form device path */
>>> - short_dp = efi_dp_shorten(device_path);
>>> - if (short_dp)
>>> - device_path = short_dp;
>>> + short_dp = efi_dp_shorten(full_path);
>>> + device_path = short_dp ? short_dp : full_path;
>>>
>>> lo.label = dev_name;
>>> lo.attributes = LOAD_OPTION_ACTIVE;
>>> @@ -396,24 +426,35 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
>>> lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
>>> /*
>>> * Set the dedicated guid to optional_data, it is used to identify
>>> - * the boot option that automatically generated by the bootmenu.
>>> + * the boot option that automatically generated by the efibootmgr.
>>> * efi_serialize_load_option() expects optional_data is null-terminated
>>> * utf8 string, so set the "1234567" string to allocate enough space
>>> * 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) {
>>> - ret = EFI_OUT_OF_RESOURCES;
>>> - goto out;
>>> + opt[num].size = efi_serialize_load_option(&lo, (u8 **)&opt[num].lo);
>>> + if (!opt[num].size) {
>>> + efi_free_pool(full_path);
>>> + efi_free_pool(dp);
>>> + efi_free_pool(fp);
>>> + efi_free_pool(default_file_path);
>>> + return EFI_OUT_OF_RESOURCES;
>>> }
>>> /* 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++;
>>> +
>>> +next_entry:
>>> + efi_free_pool(full_path);
>>> + efi_free_pool(dp);
>>> + efi_free_pool(fp);
>>> + efi_free_pool(default_file_path);
>>> }
>>>
>>> -out:
>>> - return ret;
>>> + *count = num;
>>> +
>>> + return EFI_SUCCESS;
>>> }
>>>
>>> /**
>>> @@ -642,7 +683,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.
>>> + * and create the boot option with default file if the file exists.
>>> * This function also provide the BOOT#### variable maintenance for
>>> * the media device entries.
>>> * - Automatically create the BOOT#### variable for the newly detected device,
>>> @@ -674,8 +715,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;
>>>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-16 12:46 ` Heinrich Schuchardt
@ 2023-10-16 12:57 ` Ilias Apalodimas
2023-10-16 13:00 ` Masahisa Kojima
1 sibling, 0 replies; 33+ messages in thread
From: Ilias Apalodimas @ 2023-10-16 12:57 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Masahisa Kojima, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
On Mon, Oct 16, 2023 at 02:46:46PM +0200, Heinrich Schuchardt wrote:
> On 16.10.23 14:31, Ilias Apalodimas wrote:
> > Hi Heinrich,
> >
> > On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > >
> > >
> > > Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
> > > > Current efibootmgr automatically creates the
> > > > boot options of all disks and partitions installing
> > > > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> > > > Some of the automatically created boot options are
> > > > useless if the disk and partition does not have
> > > > the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > > >
> > > > This commit only creates the boot option if the disk and
> > > > partition have the default file so that system can directly
> > > > boot from it.
> > >
> > > I don't directly see the user benefit.
> >
> > The user can add an HTTP boot option now and the installer will
> > automatically start. That would allow products to ship with a single
> > boot option provisioned and run an installer on first boot
>
> This commit is not about HTTP. It changes how boot options for block
> devices are created.
>
You are right, I misread the original reply. We should keep those as well
[...]
Thanks
/Ilias
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-16 12:46 ` Heinrich Schuchardt
2023-10-16 12:57 ` Ilias Apalodimas
@ 2023-10-16 13:00 ` Masahisa Kojima
2023-10-16 14:47 ` Heinrich Schuchardt
1 sibling, 1 reply; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-16 13:00 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Ilias Apalodimas, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
On Mon, 16 Oct 2023 at 21:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 16.10.23 14:31, Ilias Apalodimas wrote:
> > Hi Heinrich,
> >
> > On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >>
> >>
> >> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
> >>> Current efibootmgr automatically creates the
> >>> boot options of all disks and partitions installing
> >>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> >>> Some of the automatically created boot options are
> >>> useless if the disk and partition does not have
> >>> the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> >>>
> >>> This commit only creates the boot option if the disk and
> >>> partition have the default file so that system can directly
> >>> boot from it.
> >>
> >> I don't directly see the user benefit.
> >
> > The user can add an HTTP boot option now and the installer will
> > automatically start. That would allow products to ship with a single
> > boot option provisioned and run an installer on first boot
>
> This commit is not about HTTP. It changes how boot options for block
> devices are created.
>
> >
> >>
> >> Reading all file systems will increase the boot time. Shouldn't we avoid this?
> >
> > Any idea what would be an alternative? But when we added the
> > automatic boot options we said we should avoid dynamic scanning and
> > store results in a file. This is doing a similar thing. The only
> > difference is that it mounts the iso image before adding the boot
> > option.
>
> The alternative is to keep showing boot options for block devices even
> if there is no BOOTxxxx.EFI file.
>
> >
> >>
> >> What does EDK II do?
> >
> > No Idea :)
>
>
> On my workstation I get generated boot options
>
> Boot0001* UEFI:CD/DVD Drive BBS(129,,0x0)
> Boot0003* UEFI:Removable Device BBS(130,,0x0)
> Boot0004* UEFI:Network Device BBS(131,,0x0)
>
> without any media inserted and without any PXE server available.
It is just information about how the EDK2 works.
When I attach the Fedora installation media on EDK2(OVMF),
the automatically generated boot option is as follows.
UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
When this boot option is selected, Fedora installer automatically starts.
So EDK II is searching the default file on the fly.
Thanks,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> >
> > Thanks
> > /Ilias
> >>
> >> Does the UEFI specification warrant this change?
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>> ---
> >>> lib/efi_loader/efi_bootmgr.c | 86 ++++++++++++++++++++++++++----------
> >>> 1 file changed, 63 insertions(+), 23 deletions(-)
> >>>
> >>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>> index a40762c74c..c8cf1c5506 100644
> >>> --- a/lib/efi_loader/efi_bootmgr.c
> >>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>> @@ -355,40 +355,70 @@ 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;
> >>> struct efi_load_option lo;
> >>> char buf[BOOTMENU_DEVICE_NAME_MAX];
> >>> - struct efi_device_path *device_path;
> >>> + struct efi_device_path *device_path, *full_path, *dp, *fp;
> >>> struct efi_device_path *short_dp;
> >>> + struct efi_file_handle *root, *f;
> >>> + struct efi_simple_file_system_protocol *file_system;
> >>> + u16 *default_file_path = NULL;
> >>>
> >>> - ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
> >>> + ret = efi_search_protocol(volume_handles[i],
> >>> + &efi_guid_device_path, &handler);
> >>> if (ret != EFI_SUCCESS)
> >>> continue;
> >>> - ret = efi_protocol_open(handler, (void **)&device_path,
> >>> - efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >>> +
> >>> + device_path = handler->protocol_interface;
> >>> + full_path = efi_dp_from_file(device_path,
> >>> + "/EFI/BOOT/" BOOTEFI_NAME);
> >>> +
> >>> + /* check whether the partition or disk have the default file */
> >>> + ret = efi_dp_split_file_path(full_path, &dp, &fp);
> >>> + if (ret != EFI_SUCCESS || !fp)
> >>> + goto next_entry;
> >>> +
> >>> + default_file_path = efi_dp_str(fp);
> >>> + if (!default_file_path)
> >>> + goto next_entry;
> >>> +
> >>> + ret = efi_search_protocol(volume_handles[i],
> >>> + &efi_simple_file_system_protocol_guid,
> >>> + &handler);
> >>> if (ret != EFI_SUCCESS)
> >>> - continue;
> >>> + goto next_entry;
> >>> +
> >>> + file_system = handler->protocol_interface;
> >>> + ret = EFI_CALL(file_system->open_volume(file_system, &root));
> >>> + if (ret != EFI_SUCCESS)
> >>> + goto next_entry;
> >>> +
> >>> + ret = EFI_CALL(root->open(root, &f, default_file_path,
> >>> + EFI_FILE_MODE_READ, 0));
> >>> + if (ret != EFI_SUCCESS)
> >>> + goto next_entry;
> >>> +
> >>> + EFI_CALL(f->close(f));
> >>>
> >>> ret = efi_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
> >>> if (ret != EFI_SUCCESS)
> >>> - continue;
> >>> + goto next_entry;
> >>>
> >>> p = dev_name;
> >>> utf8_utf16_strncpy(&p, buf, strlen(buf));
> >>>
> >>> /* prefer to short form device path */
> >>> - short_dp = efi_dp_shorten(device_path);
> >>> - if (short_dp)
> >>> - device_path = short_dp;
> >>> + short_dp = efi_dp_shorten(full_path);
> >>> + device_path = short_dp ? short_dp : full_path;
> >>>
> >>> lo.label = dev_name;
> >>> lo.attributes = LOAD_OPTION_ACTIVE;
> >>> @@ -396,24 +426,35 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
> >>> lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
> >>> /*
> >>> * Set the dedicated guid to optional_data, it is used to identify
> >>> - * the boot option that automatically generated by the bootmenu.
> >>> + * the boot option that automatically generated by the efibootmgr.
> >>> * efi_serialize_load_option() expects optional_data is null-terminated
> >>> * utf8 string, so set the "1234567" string to allocate enough space
> >>> * 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) {
> >>> - ret = EFI_OUT_OF_RESOURCES;
> >>> - goto out;
> >>> + opt[num].size = efi_serialize_load_option(&lo, (u8 **)&opt[num].lo);
> >>> + if (!opt[num].size) {
> >>> + efi_free_pool(full_path);
> >>> + efi_free_pool(dp);
> >>> + efi_free_pool(fp);
> >>> + efi_free_pool(default_file_path);
> >>> + return EFI_OUT_OF_RESOURCES;
> >>> }
> >>> /* 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++;
> >>> +
> >>> +next_entry:
> >>> + efi_free_pool(full_path);
> >>> + efi_free_pool(dp);
> >>> + efi_free_pool(fp);
> >>> + efi_free_pool(default_file_path);
> >>> }
> >>>
> >>> -out:
> >>> - return ret;
> >>> + *count = num;
> >>> +
> >>> + return EFI_SUCCESS;
> >>> }
> >>>
> >>> /**
> >>> @@ -642,7 +683,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.
> >>> + * and create the boot option with default file if the file exists.
> >>> * This function also provide the BOOT#### variable maintenance for
> >>> * the media device entries.
> >>> * - Automatically create the BOOT#### variable for the newly detected device,
> >>> @@ -674,8 +715,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;
> >>>
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-16 13:00 ` Masahisa Kojima
@ 2023-10-16 14:47 ` Heinrich Schuchardt
2023-10-17 5:32 ` Masahisa Kojima
0 siblings, 1 reply; 33+ messages in thread
From: Heinrich Schuchardt @ 2023-10-16 14:47 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Ilias Apalodimas, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
On 16.10.23 15:00, Masahisa Kojima wrote:
> On Mon, 16 Oct 2023 at 21:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 16.10.23 14:31, Ilias Apalodimas wrote:
>>> Hi Heinrich,
>>>
>>> On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>>
>>>>
>>>> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
>>>>> Current efibootmgr automatically creates the
>>>>> boot options of all disks and partitions installing
>>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
>>>>> Some of the automatically created boot options are
>>>>> useless if the disk and partition does not have
>>>>> the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
>>>>>
>>>>> This commit only creates the boot option if the disk and
>>>>> partition have the default file so that system can directly
>>>>> boot from it.
>>>>
>>>> I don't directly see the user benefit.
>>>
>>> The user can add an HTTP boot option now and the installer will
>>> automatically start. That would allow products to ship with a single
>>> boot option provisioned and run an installer on first boot
>>
>> This commit is not about HTTP. It changes how boot options for block
>> devices are created.
>>
>>>
>>>>
>>>> Reading all file systems will increase the boot time. Shouldn't we avoid this?
>>>
>>> Any idea what would be an alternative? But when we added the
>>> automatic boot options we said we should avoid dynamic scanning and
>>> store results in a file. This is doing a similar thing. The only
>>> difference is that it mounts the iso image before adding the boot
>>> option.
>>
>> The alternative is to keep showing boot options for block devices even
>> if there is no BOOTxxxx.EFI file.
>>
>>>
>>>>
>>>> What does EDK II do?
>>>
>>> No Idea :)
>>
>>
>> On my workstation I get generated boot options
>>
>> Boot0001* UEFI:CD/DVD Drive BBS(129,,0x0)
>> Boot0003* UEFI:Removable Device BBS(130,,0x0)
>> Boot0004* UEFI:Network Device BBS(131,,0x0)
>>
>> without any media inserted and without any PXE server available.
>
> It is just information about how the EDK2 works.
> When I attach the Fedora installation media on EDK2(OVMF),
> the automatically generated boot option is as follows.
>
> UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
An ATAPI drive typically is not removable. So I wonder why it is listed.
Did you set the removable flag on the command line?
>
> When this boot option is selected, Fedora installer automatically starts.
> So EDK II is searching the default file on the fly.
What is shown if you attach a medium without Bootaa64.efi?
Best regards
Heinrich
>
> Thanks,
> Masahisa Kojima
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Thanks
>>> /Ilias
>>>>
>>>> Does the UEFI specification warrant this change?
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>>> ---
>>>>> lib/efi_loader/efi_bootmgr.c | 86 ++++++++++++++++++++++++++----------
>>>>> 1 file changed, 63 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>>>> index a40762c74c..c8cf1c5506 100644
>>>>> --- a/lib/efi_loader/efi_bootmgr.c
>>>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>>>> @@ -355,40 +355,70 @@ 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;
>>>>> struct efi_load_option lo;
>>>>> char buf[BOOTMENU_DEVICE_NAME_MAX];
>>>>> - struct efi_device_path *device_path;
>>>>> + struct efi_device_path *device_path, *full_path, *dp, *fp;
>>>>> struct efi_device_path *short_dp;
>>>>> + struct efi_file_handle *root, *f;
>>>>> + struct efi_simple_file_system_protocol *file_system;
>>>>> + u16 *default_file_path = NULL;
>>>>>
>>>>> - ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
>>>>> + ret = efi_search_protocol(volume_handles[i],
>>>>> + &efi_guid_device_path, &handler);
>>>>> if (ret != EFI_SUCCESS)
>>>>> continue;
>>>>> - ret = efi_protocol_open(handler, (void **)&device_path,
>>>>> - efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>>>> +
>>>>> + device_path = handler->protocol_interface;
>>>>> + full_path = efi_dp_from_file(device_path,
>>>>> + "/EFI/BOOT/" BOOTEFI_NAME);
>>>>> +
>>>>> + /* check whether the partition or disk have the default file */
>>>>> + ret = efi_dp_split_file_path(full_path, &dp, &fp);
>>>>> + if (ret != EFI_SUCCESS || !fp)
>>>>> + goto next_entry;
>>>>> +
>>>>> + default_file_path = efi_dp_str(fp);
>>>>> + if (!default_file_path)
>>>>> + goto next_entry;
>>>>> +
>>>>> + ret = efi_search_protocol(volume_handles[i],
>>>>> + &efi_simple_file_system_protocol_guid,
>>>>> + &handler);
>>>>> if (ret != EFI_SUCCESS)
>>>>> - continue;
>>>>> + goto next_entry;
>>>>> +
>>>>> + file_system = handler->protocol_interface;
>>>>> + ret = EFI_CALL(file_system->open_volume(file_system, &root));
>>>>> + if (ret != EFI_SUCCESS)
>>>>> + goto next_entry;
>>>>> +
>>>>> + ret = EFI_CALL(root->open(root, &f, default_file_path,
>>>>> + EFI_FILE_MODE_READ, 0));
>>>>> + if (ret != EFI_SUCCESS)
>>>>> + goto next_entry;
>>>>> +
>>>>> + EFI_CALL(f->close(f));
>>>>>
>>>>> ret = efi_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
>>>>> if (ret != EFI_SUCCESS)
>>>>> - continue;
>>>>> + goto next_entry;
>>>>>
>>>>> p = dev_name;
>>>>> utf8_utf16_strncpy(&p, buf, strlen(buf));
>>>>>
>>>>> /* prefer to short form device path */
>>>>> - short_dp = efi_dp_shorten(device_path);
>>>>> - if (short_dp)
>>>>> - device_path = short_dp;
>>>>> + short_dp = efi_dp_shorten(full_path);
>>>>> + device_path = short_dp ? short_dp : full_path;
>>>>>
>>>>> lo.label = dev_name;
>>>>> lo.attributes = LOAD_OPTION_ACTIVE;
>>>>> @@ -396,24 +426,35 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
>>>>> lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
>>>>> /*
>>>>> * Set the dedicated guid to optional_data, it is used to identify
>>>>> - * the boot option that automatically generated by the bootmenu.
>>>>> + * the boot option that automatically generated by the efibootmgr.
>>>>> * efi_serialize_load_option() expects optional_data is null-terminated
>>>>> * utf8 string, so set the "1234567" string to allocate enough space
>>>>> * 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) {
>>>>> - ret = EFI_OUT_OF_RESOURCES;
>>>>> - goto out;
>>>>> + opt[num].size = efi_serialize_load_option(&lo, (u8 **)&opt[num].lo);
>>>>> + if (!opt[num].size) {
>>>>> + efi_free_pool(full_path);
>>>>> + efi_free_pool(dp);
>>>>> + efi_free_pool(fp);
>>>>> + efi_free_pool(default_file_path);
>>>>> + return EFI_OUT_OF_RESOURCES;
>>>>> }
>>>>> /* 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++;
>>>>> +
>>>>> +next_entry:
>>>>> + efi_free_pool(full_path);
>>>>> + efi_free_pool(dp);
>>>>> + efi_free_pool(fp);
>>>>> + efi_free_pool(default_file_path);
>>>>> }
>>>>>
>>>>> -out:
>>>>> - return ret;
>>>>> + *count = num;
>>>>> +
>>>>> + return EFI_SUCCESS;
>>>>> }
>>>>>
>>>>> /**
>>>>> @@ -642,7 +683,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.
>>>>> + * and create the boot option with default file if the file exists.
>>>>> * This function also provide the BOOT#### variable maintenance for
>>>>> * the media device entries.
>>>>> * - Automatically create the BOOT#### variable for the newly detected device,
>>>>> @@ -674,8 +715,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;
>>>>>
>>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-16 14:47 ` Heinrich Schuchardt
@ 2023-10-17 5:32 ` Masahisa Kojima
2023-10-17 19:46 ` Ilias Apalodimas
2023-10-17 20:29 ` Heinrich Schuchardt
0 siblings, 2 replies; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-17 5:32 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Ilias Apalodimas, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
Hi Heinrich,
On Mon, 16 Oct 2023 at 23:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 16.10.23 15:00, Masahisa Kojima wrote:
> > On Mon, 16 Oct 2023 at 21:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 16.10.23 14:31, Ilias Apalodimas wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>>
> >>>>
> >>>> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
> >>>>> Current efibootmgr automatically creates the
> >>>>> boot options of all disks and partitions installing
> >>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> >>>>> Some of the automatically created boot options are
> >>>>> useless if the disk and partition does not have
> >>>>> the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> >>>>>
> >>>>> This commit only creates the boot option if the disk and
> >>>>> partition have the default file so that system can directly
> >>>>> boot from it.
> >>>>
> >>>> I don't directly see the user benefit.
> >>>
> >>> The user can add an HTTP boot option now and the installer will
> >>> automatically start. That would allow products to ship with a single
> >>> boot option provisioned and run an installer on first boot
> >>
> >> This commit is not about HTTP. It changes how boot options for block
> >> devices are created.
> >>
> >>>
> >>>>
> >>>> Reading all file systems will increase the boot time. Shouldn't we avoid this?
> >>>
> >>> Any idea what would be an alternative? But when we added the
> >>> automatic boot options we said we should avoid dynamic scanning and
> >>> store results in a file. This is doing a similar thing. The only
> >>> difference is that it mounts the iso image before adding the boot
> >>> option.
> >>
> >> The alternative is to keep showing boot options for block devices even
> >> if there is no BOOTxxxx.EFI file.
> >>
> >>>
> >>>>
> >>>> What does EDK II do?
> >>>
> >>> No Idea :)
> >>
> >>
> >> On my workstation I get generated boot options
> >>
> >> Boot0001* UEFI:CD/DVD Drive BBS(129,,0x0)
> >> Boot0003* UEFI:Removable Device BBS(130,,0x0)
> >> Boot0004* UEFI:Network Device BBS(131,,0x0)
> >>
> >> without any media inserted and without any PXE server available.
> >
> > It is just information about how the EDK2 works.
> > When I attach the Fedora installation media on EDK2(OVMF),
> > the automatically generated boot option is as follows.
> >
> > UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
>
> An ATAPI drive typically is not removable. So I wonder why it is listed.
> Did you set the removable flag on the command line?
I guess it is not removable(actually I don't know how to set the
device as removable).
I just attached the iso image to QEMU with something like '-hda
Fedora_netinst.iso".
According to the EDK II implementation[1], the boot option is
enumerated with the following order.
1. Removable BlockIo
2. Fixed BlockIo
3. Non-BlockIo SimpleFileSystem
4. LoadFile
So boot option for the fixed device such as HDD is also automatically created.
[1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L2150
>
> >
> > When this boot option is selected, Fedora installer automatically starts.
> > So EDK II is searching the default file on the fly.
>
> What is shown if you attach a medium without Bootaa64.efi?
The same boot option is created.
UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
Thanks,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> >
> > Thanks,
> > Masahisa Kojima
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Thanks
> >>> /Ilias
> >>>>
> >>>> Does the UEFI specification warrant this change?
> >>>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>>
> >>>>>
> >>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> >>>>> ---
> >>>>> lib/efi_loader/efi_bootmgr.c | 86 ++++++++++++++++++++++++++----------
> >>>>> 1 file changed, 63 insertions(+), 23 deletions(-)
> >>>>>
> >>>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> >>>>> index a40762c74c..c8cf1c5506 100644
> >>>>> --- a/lib/efi_loader/efi_bootmgr.c
> >>>>> +++ b/lib/efi_loader/efi_bootmgr.c
> >>>>> @@ -355,40 +355,70 @@ 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;
> >>>>> struct efi_load_option lo;
> >>>>> char buf[BOOTMENU_DEVICE_NAME_MAX];
> >>>>> - struct efi_device_path *device_path;
> >>>>> + struct efi_device_path *device_path, *full_path, *dp, *fp;
> >>>>> struct efi_device_path *short_dp;
> >>>>> + struct efi_file_handle *root, *f;
> >>>>> + struct efi_simple_file_system_protocol *file_system;
> >>>>> + u16 *default_file_path = NULL;
> >>>>>
> >>>>> - ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
> >>>>> + ret = efi_search_protocol(volume_handles[i],
> >>>>> + &efi_guid_device_path, &handler);
> >>>>> if (ret != EFI_SUCCESS)
> >>>>> continue;
> >>>>> - ret = efi_protocol_open(handler, (void **)&device_path,
> >>>>> - efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> >>>>> +
> >>>>> + device_path = handler->protocol_interface;
> >>>>> + full_path = efi_dp_from_file(device_path,
> >>>>> + "/EFI/BOOT/" BOOTEFI_NAME);
> >>>>> +
> >>>>> + /* check whether the partition or disk have the default file */
> >>>>> + ret = efi_dp_split_file_path(full_path, &dp, &fp);
> >>>>> + if (ret != EFI_SUCCESS || !fp)
> >>>>> + goto next_entry;
> >>>>> +
> >>>>> + default_file_path = efi_dp_str(fp);
> >>>>> + if (!default_file_path)
> >>>>> + goto next_entry;
> >>>>> +
> >>>>> + ret = efi_search_protocol(volume_handles[i],
> >>>>> + &efi_simple_file_system_protocol_guid,
> >>>>> + &handler);
> >>>>> if (ret != EFI_SUCCESS)
> >>>>> - continue;
> >>>>> + goto next_entry;
> >>>>> +
> >>>>> + file_system = handler->protocol_interface;
> >>>>> + ret = EFI_CALL(file_system->open_volume(file_system, &root));
> >>>>> + if (ret != EFI_SUCCESS)
> >>>>> + goto next_entry;
> >>>>> +
> >>>>> + ret = EFI_CALL(root->open(root, &f, default_file_path,
> >>>>> + EFI_FILE_MODE_READ, 0));
> >>>>> + if (ret != EFI_SUCCESS)
> >>>>> + goto next_entry;
> >>>>> +
> >>>>> + EFI_CALL(f->close(f));
> >>>>>
> >>>>> ret = efi_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
> >>>>> if (ret != EFI_SUCCESS)
> >>>>> - continue;
> >>>>> + goto next_entry;
> >>>>>
> >>>>> p = dev_name;
> >>>>> utf8_utf16_strncpy(&p, buf, strlen(buf));
> >>>>>
> >>>>> /* prefer to short form device path */
> >>>>> - short_dp = efi_dp_shorten(device_path);
> >>>>> - if (short_dp)
> >>>>> - device_path = short_dp;
> >>>>> + short_dp = efi_dp_shorten(full_path);
> >>>>> + device_path = short_dp ? short_dp : full_path;
> >>>>>
> >>>>> lo.label = dev_name;
> >>>>> lo.attributes = LOAD_OPTION_ACTIVE;
> >>>>> @@ -396,24 +426,35 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
> >>>>> lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
> >>>>> /*
> >>>>> * Set the dedicated guid to optional_data, it is used to identify
> >>>>> - * the boot option that automatically generated by the bootmenu.
> >>>>> + * the boot option that automatically generated by the efibootmgr.
> >>>>> * efi_serialize_load_option() expects optional_data is null-terminated
> >>>>> * utf8 string, so set the "1234567" string to allocate enough space
> >>>>> * 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) {
> >>>>> - ret = EFI_OUT_OF_RESOURCES;
> >>>>> - goto out;
> >>>>> + opt[num].size = efi_serialize_load_option(&lo, (u8 **)&opt[num].lo);
> >>>>> + if (!opt[num].size) {
> >>>>> + efi_free_pool(full_path);
> >>>>> + efi_free_pool(dp);
> >>>>> + efi_free_pool(fp);
> >>>>> + efi_free_pool(default_file_path);
> >>>>> + return EFI_OUT_OF_RESOURCES;
> >>>>> }
> >>>>> /* 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++;
> >>>>> +
> >>>>> +next_entry:
> >>>>> + efi_free_pool(full_path);
> >>>>> + efi_free_pool(dp);
> >>>>> + efi_free_pool(fp);
> >>>>> + efi_free_pool(default_file_path);
> >>>>> }
> >>>>>
> >>>>> -out:
> >>>>> - return ret;
> >>>>> + *count = num;
> >>>>> +
> >>>>> + return EFI_SUCCESS;
> >>>>> }
> >>>>>
> >>>>> /**
> >>>>> @@ -642,7 +683,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.
> >>>>> + * and create the boot option with default file if the file exists.
> >>>>> * This function also provide the BOOT#### variable maintenance for
> >>>>> * the media device entries.
> >>>>> * - Automatically create the BOOT#### variable for the newly detected device,
> >>>>> @@ -674,8 +715,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;
> >>>>>
> >>
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-17 5:32 ` Masahisa Kojima
@ 2023-10-17 19:46 ` Ilias Apalodimas
2023-10-18 9:07 ` Masahisa Kojima
2023-10-17 20:29 ` Heinrich Schuchardt
1 sibling, 1 reply; 33+ messages in thread
From: Ilias Apalodimas @ 2023-10-17 19:46 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Heinrich Schuchardt, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
Hi all,
On Tue, 17 Oct 2023 at 08:33, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Hi Heinrich,
>
> On Mon, 16 Oct 2023 at 23:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 16.10.23 15:00, Masahisa Kojima wrote:
> > > On Mon, 16 Oct 2023 at 21:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >>
> > >> On 16.10.23 14:31, Ilias Apalodimas wrote:
> > >>> Hi Heinrich,
> > >>>
> > >>> On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >>>>
> > >>>>
> > >>>>
> > >>>> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
> > >>>>> Current efibootmgr automatically creates the
> > >>>>> boot options of all disks and partitions installing
> > >>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> > >>>>> Some of the automatically created boot options are
> > >>>>> useless if the disk and partition does not have
> > >>>>> the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > >>>>>
> > >>>>> This commit only creates the boot option if the disk and
> > >>>>> partition have the default file so that system can directly
> > >>>>> boot from it.
> > >>>>
> > >>>> I don't directly see the user benefit.
> > >>>
> > >>> The user can add an HTTP boot option now and the installer will
> > >>> automatically start. That would allow products to ship with a single
> > >>> boot option provisioned and run an installer on first boot
> > >>
> > >> This commit is not about HTTP. It changes how boot options for block
> > >> devices are created.
> > >>
> > >>>
> > >>>>
> > >>>> Reading all file systems will increase the boot time. Shouldn't we avoid this?
> > >>>
> > >>> Any idea what would be an alternative? But when we added the
> > >>> automatic boot options we said we should avoid dynamic scanning and
> > >>> store results in a file. This is doing a similar thing. The only
> > >>> difference is that it mounts the iso image before adding the boot
> > >>> option.
> > >>
> > >> The alternative is to keep showing boot options for block devices even
> > >> if there is no BOOTxxxx.EFI file.
> > >>
> > >>>
> > >>>>
> > >>>> What does EDK II do?
> > >>>
> > >>> No Idea :)
> > >>
> > >>
> > >> On my workstation I get generated boot options
> > >>
> > >> Boot0001* UEFI:CD/DVD Drive BBS(129,,0x0)
> > >> Boot0003* UEFI:Removable Device BBS(130,,0x0)
> > >> Boot0004* UEFI:Network Device BBS(131,,0x0)
> > >>
> > >> without any media inserted and without any PXE server available.
> > >
> > > It is just information about how the EDK2 works.
> > > When I attach the Fedora installation media on EDK2(OVMF),
> > > the automatically generated boot option is as follows.
> > >
> > > UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
> >
> > An ATAPI drive typically is not removable. So I wonder why it is listed.
> > Did you set the removable flag on the command line?
>
> I guess it is not removable(actually I don't know how to set the
> device as removable).
> I just attached the iso image to QEMU with something like '-hda
> Fedora_netinst.iso".
>
> According to the EDK II implementation[1], the boot option is
> enumerated with the following order.
> 1. Removable BlockIo
> 2. Fixed BlockIo
> 3. Non-BlockIo SimpleFileSystem
> 4. LoadFile
> So boot option for the fixed device such as HDD is also automatically created.
>
> [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L2150
>
> >
> > >
> > > When this boot option is selected, Fedora installer automatically starts.
> > > So EDK II is searching the default file on the fly.
> >
> > What is shown if you attach a medium without Bootaa64.efi?
>
> The same boot option is created.
> UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
I went back to reading the spec and I think Heinrich is right. We
don't need that check at all. Going through [0] paragraph 4 says
" This search occurs when the device path of the boot image listed in
any boot option points directly to an EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
device and does not specify the exact file to load"
So we should *only* add an automatic variable without the default
application. Our code in try_load_entry() will search for that
[0] https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html#load-option-processing
Regards
/Ilias
>
> Thanks,
> Masahisa Kojima
>
> >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-17 19:46 ` Ilias Apalodimas
@ 2023-10-18 9:07 ` Masahisa Kojima
2023-10-18 9:12 ` Ilias Apalodimas
0 siblings, 1 reply; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-18 9:07 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Heinrich Schuchardt, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
Hi Ilias,
On Wed, 18 Oct 2023 at 04:47, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi all,
>
> On Tue, 17 Oct 2023 at 08:33, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > Hi Heinrich,
> >
> > On Mon, 16 Oct 2023 at 23:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 16.10.23 15:00, Masahisa Kojima wrote:
> > > > On Mon, 16 Oct 2023 at 21:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >>
> > > >> On 16.10.23 14:31, Ilias Apalodimas wrote:
> > > >>> Hi Heinrich,
> > > >>>
> > > >>> On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >>>>
> > > >>>>
> > > >>>>
> > > >>>> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
> > > >>>>> Current efibootmgr automatically creates the
> > > >>>>> boot options of all disks and partitions installing
> > > >>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> > > >>>>> Some of the automatically created boot options are
> > > >>>>> useless if the disk and partition does not have
> > > >>>>> the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > > >>>>>
> > > >>>>> This commit only creates the boot option if the disk and
> > > >>>>> partition have the default file so that system can directly
> > > >>>>> boot from it.
> > > >>>>
> > > >>>> I don't directly see the user benefit.
> > > >>>
> > > >>> The user can add an HTTP boot option now and the installer will
> > > >>> automatically start. That would allow products to ship with a single
> > > >>> boot option provisioned and run an installer on first boot
> > > >>
> > > >> This commit is not about HTTP. It changes how boot options for block
> > > >> devices are created.
> > > >>
> > > >>>
> > > >>>>
> > > >>>> Reading all file systems will increase the boot time. Shouldn't we avoid this?
> > > >>>
> > > >>> Any idea what would be an alternative? But when we added the
> > > >>> automatic boot options we said we should avoid dynamic scanning and
> > > >>> store results in a file. This is doing a similar thing. The only
> > > >>> difference is that it mounts the iso image before adding the boot
> > > >>> option.
> > > >>
> > > >> The alternative is to keep showing boot options for block devices even
> > > >> if there is no BOOTxxxx.EFI file.
> > > >>
> > > >>>
> > > >>>>
> > > >>>> What does EDK II do?
> > > >>>
> > > >>> No Idea :)
> > > >>
> > > >>
> > > >> On my workstation I get generated boot options
> > > >>
> > > >> Boot0001* UEFI:CD/DVD Drive BBS(129,,0x0)
> > > >> Boot0003* UEFI:Removable Device BBS(130,,0x0)
> > > >> Boot0004* UEFI:Network Device BBS(131,,0x0)
> > > >>
> > > >> without any media inserted and without any PXE server available.
> > > >
> > > > It is just information about how the EDK2 works.
> > > > When I attach the Fedora installation media on EDK2(OVMF),
> > > > the automatically generated boot option is as follows.
> > > >
> > > > UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
> > >
> > > An ATAPI drive typically is not removable. So I wonder why it is listed.
> > > Did you set the removable flag on the command line?
> >
> > I guess it is not removable(actually I don't know how to set the
> > device as removable).
> > I just attached the iso image to QEMU with something like '-hda
> > Fedora_netinst.iso".
> >
> > According to the EDK II implementation[1], the boot option is
> > enumerated with the following order.
> > 1. Removable BlockIo
> > 2. Fixed BlockIo
> > 3. Non-BlockIo SimpleFileSystem
> > 4. LoadFile
> > So boot option for the fixed device such as HDD is also automatically created.
> >
> > [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L2150
> >
> > >
> > > >
> > > > When this boot option is selected, Fedora installer automatically starts.
> > > > So EDK II is searching the default file on the fly.
> > >
> > > What is shown if you attach a medium without Bootaa64.efi?
> >
> > The same boot option is created.
> > UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
>
> I went back to reading the spec and I think Heinrich is right. We
> don't need that check at all. Going through [0] paragraph 4 says
> " This search occurs when the device path of the boot image listed in
> any boot option points directly to an EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> device and does not specify the exact file to load"
>
> So we should *only* add an automatic variable without the default
> application. Our code in try_load_entry() will search for that
>
Thank you for checking the UEFI specification and sorry for
overlooking the above.
So we will go back to the previous on the fly default application search.
Thanks,
Masahisa Kojima
> [0] https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html#load-option-processing
>
> Regards
> /Ilias
> >
> > Thanks,
> > Masahisa Kojima
> >
> > >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-18 9:07 ` Masahisa Kojima
@ 2023-10-18 9:12 ` Ilias Apalodimas
2023-10-18 9:30 ` Heinrich Schuchardt
2023-10-19 6:45 ` Masahisa Kojima
0 siblings, 2 replies; 33+ messages in thread
From: Ilias Apalodimas @ 2023-10-18 9:12 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Heinrich Schuchardt, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
Hi Kojima-san
On Wed, 18 Oct 2023 at 12:07, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Hi Ilias,
>
> On Wed, 18 Oct 2023 at 04:47, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hi all,
> >
> > On Tue, 17 Oct 2023 at 08:33, Masahisa Kojima
> > <masahisa.kojima@linaro.org> wrote:
> > >
> > > Hi Heinrich,
> > >
> > > On Mon, 16 Oct 2023 at 23:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > > On 16.10.23 15:00, Masahisa Kojima wrote:
> > > > > On Mon, 16 Oct 2023 at 21:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >>
> > > > >> On 16.10.23 14:31, Ilias Apalodimas wrote:
> > > > >>> Hi Heinrich,
> > > > >>>
> > > > >>> On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >>>>
> > > > >>>>
> > > > >>>>
> > > > >>>> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
> > > > >>>>> Current efibootmgr automatically creates the
> > > > >>>>> boot options of all disks and partitions installing
> > > > >>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> > > > >>>>> Some of the automatically created boot options are
> > > > >>>>> useless if the disk and partition does not have
> > > > >>>>> the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > > > >>>>>
> > > > >>>>> This commit only creates the boot option if the disk and
> > > > >>>>> partition have the default file so that system can directly
> > > > >>>>> boot from it.
> > > > >>>>
> > > > >>>> I don't directly see the user benefit.
> > > > >>>
> > > > >>> The user can add an HTTP boot option now and the installer will
> > > > >>> automatically start. That would allow products to ship with a single
> > > > >>> boot option provisioned and run an installer on first boot
> > > > >>
> > > > >> This commit is not about HTTP. It changes how boot options for block
> > > > >> devices are created.
> > > > >>
> > > > >>>
> > > > >>>>
> > > > >>>> Reading all file systems will increase the boot time. Shouldn't we avoid this?
> > > > >>>
> > > > >>> Any idea what would be an alternative? But when we added the
> > > > >>> automatic boot options we said we should avoid dynamic scanning and
> > > > >>> store results in a file. This is doing a similar thing. The only
> > > > >>> difference is that it mounts the iso image before adding the boot
> > > > >>> option.
> > > > >>
> > > > >> The alternative is to keep showing boot options for block devices even
> > > > >> if there is no BOOTxxxx.EFI file.
> > > > >>
> > > > >>>
> > > > >>>>
> > > > >>>> What does EDK II do?
> > > > >>>
> > > > >>> No Idea :)
> > > > >>
> > > > >>
> > > > >> On my workstation I get generated boot options
> > > > >>
> > > > >> Boot0001* UEFI:CD/DVD Drive BBS(129,,0x0)
> > > > >> Boot0003* UEFI:Removable Device BBS(130,,0x0)
> > > > >> Boot0004* UEFI:Network Device BBS(131,,0x0)
> > > > >>
> > > > >> without any media inserted and without any PXE server available.
> > > > >
> > > > > It is just information about how the EDK2 works.
> > > > > When I attach the Fedora installation media on EDK2(OVMF),
> > > > > the automatically generated boot option is as follows.
> > > > >
> > > > > UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
> > > >
> > > > An ATAPI drive typically is not removable. So I wonder why it is listed.
> > > > Did you set the removable flag on the command line?
> > >
> > > I guess it is not removable(actually I don't know how to set the
> > > device as removable).
> > > I just attached the iso image to QEMU with something like '-hda
> > > Fedora_netinst.iso".
> > >
> > > According to the EDK II implementation[1], the boot option is
> > > enumerated with the following order.
> > > 1. Removable BlockIo
> > > 2. Fixed BlockIo
> > > 3. Non-BlockIo SimpleFileSystem
> > > 4. LoadFile
> > > So boot option for the fixed device such as HDD is also automatically created.
> > >
> > > [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L2150
> > >
> > > >
> > > > >
> > > > > When this boot option is selected, Fedora installer automatically starts.
> > > > > So EDK II is searching the default file on the fly.
> > > >
> > > > What is shown if you attach a medium without Bootaa64.efi?
> > >
> > > The same boot option is created.
> > > UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
> >
> > I went back to reading the spec and I think Heinrich is right. We
> > don't need that check at all. Going through [0] paragraph 4 says
> > " This search occurs when the device path of the boot image listed in
> > any boot option points directly to an EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> > device and does not specify the exact file to load"
> >
> > So we should *only* add an automatic variable without the default
> > application. Our code in try_load_entry() will search for that
> >
>
> Thank you for checking the UEFI specification and sorry for
> overlooking the above.
> So we will go back to the previous on the fly default application search.
Yes, I was about to suggest that.
The problem as I understand it that the current patch not only
disregards disks and partitions that dont have a default (i.e
bootaa64.efi) file. It also *changes* the default boot option we add,
and instead of the disk it adds a file. That is the part that's
against the spec. On top of that it changes the behavior of efi
bootmgr and we never call the expand_media_path.
So my suggestion would be
- Drop #4
- Adjust patch 5 and instead of loading the boot entry directly, scan
for the special autogenerated boot option and look for that file there
Heinrich would that work for you?
Thanks
/Ilias
>
> Thanks,
> Masahisa Kojima
>
> > [0] https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html#load-option-processing
> >
> > Regards
> > /Ilias
> > >
> > > Thanks,
> > > Masahisa Kojima
> > >
> > > >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-18 9:12 ` Ilias Apalodimas
@ 2023-10-18 9:30 ` Heinrich Schuchardt
2023-10-19 6:45 ` Masahisa Kojima
1 sibling, 0 replies; 33+ messages in thread
From: Heinrich Schuchardt @ 2023-10-18 9:30 UTC (permalink / raw)
To: Ilias Apalodimas, Masahisa Kojima
Cc: u-boot, Simon Glass, Takahiro Akashi, Michal Simek
On 10/18/23 11:12, Ilias Apalodimas wrote:
> Hi Kojima-san
>
> On Wed, 18 Oct 2023 at 12:07, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
>>
>> Hi Ilias,
>>
>> On Wed, 18 Oct 2023 at 04:47, Ilias Apalodimas
>> <ilias.apalodimas@linaro.org> wrote:
>>>
>>> Hi all,
>>>
>>> On Tue, 17 Oct 2023 at 08:33, Masahisa Kojima
>>> <masahisa.kojima@linaro.org> wrote:
>>>>
>>>> Hi Heinrich,
>>>>
>>>> On Mon, 16 Oct 2023 at 23:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>
>>>>> On 16.10.23 15:00, Masahisa Kojima wrote:
>>>>>> On Mon, 16 Oct 2023 at 21:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>
>>>>>>> On 16.10.23 14:31, Ilias Apalodimas wrote:
>>>>>>>> Hi Heinrich,
>>>>>>>>
>>>>>>>> On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
>>>>>>>>>> Current efibootmgr automatically creates the
>>>>>>>>>> boot options of all disks and partitions installing
>>>>>>>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
>>>>>>>>>> Some of the automatically created boot options are
>>>>>>>>>> useless if the disk and partition does not have
>>>>>>>>>> the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
>>>>>>>>>>
>>>>>>>>>> This commit only creates the boot option if the disk and
>>>>>>>>>> partition have the default file so that system can directly
>>>>>>>>>> boot from it.
>>>>>>>>>
>>>>>>>>> I don't directly see the user benefit.
>>>>>>>>
>>>>>>>> The user can add an HTTP boot option now and the installer will
>>>>>>>> automatically start. That would allow products to ship with a single
>>>>>>>> boot option provisioned and run an installer on first boot
>>>>>>>
>>>>>>> This commit is not about HTTP. It changes how boot options for block
>>>>>>> devices are created.
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Reading all file systems will increase the boot time. Shouldn't we avoid this?
>>>>>>>>
>>>>>>>> Any idea what would be an alternative? But when we added the
>>>>>>>> automatic boot options we said we should avoid dynamic scanning and
>>>>>>>> store results in a file. This is doing a similar thing. The only
>>>>>>>> difference is that it mounts the iso image before adding the boot
>>>>>>>> option.
>>>>>>>
>>>>>>> The alternative is to keep showing boot options for block devices even
>>>>>>> if there is no BOOTxxxx.EFI file.
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> What does EDK II do?
>>>>>>>>
>>>>>>>> No Idea :)
>>>>>>>
>>>>>>>
>>>>>>> On my workstation I get generated boot options
>>>>>>>
>>>>>>> Boot0001* UEFI:CD/DVD Drive BBS(129,,0x0)
>>>>>>> Boot0003* UEFI:Removable Device BBS(130,,0x0)
>>>>>>> Boot0004* UEFI:Network Device BBS(131,,0x0)
>>>>>>>
>>>>>>> without any media inserted and without any PXE server available.
>>>>>>
>>>>>> It is just information about how the EDK2 works.
>>>>>> When I attach the Fedora installation media on EDK2(OVMF),
>>>>>> the automatically generated boot option is as follows.
>>>>>>
>>>>>> UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
>>>>>
>>>>> An ATAPI drive typically is not removable. So I wonder why it is listed.
>>>>> Did you set the removable flag on the command line?
>>>>
>>>> I guess it is not removable(actually I don't know how to set the
>>>> device as removable).
>>>> I just attached the iso image to QEMU with something like '-hda
>>>> Fedora_netinst.iso".
>>>>
>>>> According to the EDK II implementation[1], the boot option is
>>>> enumerated with the following order.
>>>> 1. Removable BlockIo
>>>> 2. Fixed BlockIo
>>>> 3. Non-BlockIo SimpleFileSystem
>>>> 4. LoadFile
>>>> So boot option for the fixed device such as HDD is also automatically created.
>>>>
>>>> [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L2150
>>>>
>>>>>
>>>>>>
>>>>>> When this boot option is selected, Fedora installer automatically starts.
>>>>>> So EDK II is searching the default file on the fly.
>>>>>
>>>>> What is shown if you attach a medium without Bootaa64.efi?
>>>>
>>>> The same boot option is created.
>>>> UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
>>>
>>> I went back to reading the spec and I think Heinrich is right. We
>>> don't need that check at all. Going through [0] paragraph 4 says
>>> " This search occurs when the device path of the boot image listed in
>>> any boot option points directly to an EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
>>> device and does not specify the exact file to load"
>>>
>>> So we should *only* add an automatic variable without the default
>>> application. Our code in try_load_entry() will search for that
>>>
>>
>> Thank you for checking the UEFI specification and sorry for
>> overlooking the above.
>> So we will go back to the previous on the fly default application search.
>
> Yes, I was about to suggest that.
> The problem as I understand it that the current patch not only
> disregards disks and partitions that dont have a default (i.e
> bootaa64.efi) file. It also *changes* the default boot option we add,
> and instead of the disk it adds a file. That is the part that's
> against the spec. On top of that it changes the behavior of efi
> bootmgr and we never call the expand_media_path.
>
> So my suggestion would be
> - Drop #4
> - Adjust patch 5 and instead of loading the boot entry directly, scan
> for the special autogenerated boot option and look for that file there
>
> Heinrich would that work for you?
That is fine with me. This allows us to handle any deviations of U-Boot
from the spec concerning generated boot options in a separate patch series.
We still need to look into adding the deletion of the blkmap resource in
case of error or when the EFI application returns.
Best regards
Heinrich
>
> Thanks
> /Ilias
>>
>> Thanks,
>> Masahisa Kojima
>>
>>> [0] https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html#load-option-processing
>>>
>>> Regards
>>> /Ilias
>>>>
>>>> Thanks,
>>>> Masahisa Kojima
>>>>
>>>>>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-18 9:12 ` Ilias Apalodimas
2023-10-18 9:30 ` Heinrich Schuchardt
@ 2023-10-19 6:45 ` Masahisa Kojima
2023-10-19 7:22 ` Ilias Apalodimas
1 sibling, 1 reply; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-19 6:45 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Heinrich Schuchardt, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
Hi Ilias,
On Wed, 18 Oct 2023 at 18:12, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Kojima-san
>
> On Wed, 18 Oct 2023 at 12:07, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 18 Oct 2023 at 04:47, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Hi all,
> > >
> > > On Tue, 17 Oct 2023 at 08:33, Masahisa Kojima
> > > <masahisa.kojima@linaro.org> wrote:
> > > >
> > > > Hi Heinrich,
> > > >
> > > > On Mon, 16 Oct 2023 at 23:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >
> > > > > On 16.10.23 15:00, Masahisa Kojima wrote:
> > > > > > On Mon, 16 Oct 2023 at 21:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >>
> > > > > >> On 16.10.23 14:31, Ilias Apalodimas wrote:
> > > > > >>> Hi Heinrich,
> > > > > >>>
> > > > > >>> On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >>>>
> > > > > >>>>
> > > > > >>>>
> > > > > >>>> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
> > > > > >>>>> Current efibootmgr automatically creates the
> > > > > >>>>> boot options of all disks and partitions installing
> > > > > >>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
> > > > > >>>>> Some of the automatically created boot options are
> > > > > >>>>> useless if the disk and partition does not have
> > > > > >>>>> the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> > > > > >>>>>
> > > > > >>>>> This commit only creates the boot option if the disk and
> > > > > >>>>> partition have the default file so that system can directly
> > > > > >>>>> boot from it.
> > > > > >>>>
> > > > > >>>> I don't directly see the user benefit.
> > > > > >>>
> > > > > >>> The user can add an HTTP boot option now and the installer will
> > > > > >>> automatically start. That would allow products to ship with a single
> > > > > >>> boot option provisioned and run an installer on first boot
> > > > > >>
> > > > > >> This commit is not about HTTP. It changes how boot options for block
> > > > > >> devices are created.
> > > > > >>
> > > > > >>>
> > > > > >>>>
> > > > > >>>> Reading all file systems will increase the boot time. Shouldn't we avoid this?
> > > > > >>>
> > > > > >>> Any idea what would be an alternative? But when we added the
> > > > > >>> automatic boot options we said we should avoid dynamic scanning and
> > > > > >>> store results in a file. This is doing a similar thing. The only
> > > > > >>> difference is that it mounts the iso image before adding the boot
> > > > > >>> option.
> > > > > >>
> > > > > >> The alternative is to keep showing boot options for block devices even
> > > > > >> if there is no BOOTxxxx.EFI file.
> > > > > >>
> > > > > >>>
> > > > > >>>>
> > > > > >>>> What does EDK II do?
> > > > > >>>
> > > > > >>> No Idea :)
> > > > > >>
> > > > > >>
> > > > > >> On my workstation I get generated boot options
> > > > > >>
> > > > > >> Boot0001* UEFI:CD/DVD Drive BBS(129,,0x0)
> > > > > >> Boot0003* UEFI:Removable Device BBS(130,,0x0)
> > > > > >> Boot0004* UEFI:Network Device BBS(131,,0x0)
> > > > > >>
> > > > > >> without any media inserted and without any PXE server available.
> > > > > >
> > > > > > It is just information about how the EDK2 works.
> > > > > > When I attach the Fedora installation media on EDK2(OVMF),
> > > > > > the automatically generated boot option is as follows.
> > > > > >
> > > > > > UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
> > > > >
> > > > > An ATAPI drive typically is not removable. So I wonder why it is listed.
> > > > > Did you set the removable flag on the command line?
> > > >
> > > > I guess it is not removable(actually I don't know how to set the
> > > > device as removable).
> > > > I just attached the iso image to QEMU with something like '-hda
> > > > Fedora_netinst.iso".
> > > >
> > > > According to the EDK II implementation[1], the boot option is
> > > > enumerated with the following order.
> > > > 1. Removable BlockIo
> > > > 2. Fixed BlockIo
> > > > 3. Non-BlockIo SimpleFileSystem
> > > > 4. LoadFile
> > > > So boot option for the fixed device such as HDD is also automatically created.
> > > >
> > > > [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L2150
> > > >
> > > > >
> > > > > >
> > > > > > When this boot option is selected, Fedora installer automatically starts.
> > > > > > So EDK II is searching the default file on the fly.
> > > > >
> > > > > What is shown if you attach a medium without Bootaa64.efi?
> > > >
> > > > The same boot option is created.
> > > > UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
> > >
> > > I went back to reading the spec and I think Heinrich is right. We
> > > don't need that check at all. Going through [0] paragraph 4 says
> > > " This search occurs when the device path of the boot image listed in
> > > any boot option points directly to an EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> > > device and does not specify the exact file to load"
> > >
> > > So we should *only* add an automatic variable without the default
> > > application. Our code in try_load_entry() will search for that
> > >
> >
> > Thank you for checking the UEFI specification and sorry for
> > overlooking the above.
> > So we will go back to the previous on the fly default application search.
>
> Yes, I was about to suggest that.
> The problem as I understand it that the current patch not only
> disregards disks and partitions that dont have a default (i.e
> bootaa64.efi) file. It also *changes* the default boot option we add,
> and instead of the disk it adds a file. That is the part that's
> against the spec. On top of that it changes the behavior of efi
> bootmgr and we never call the expand_media_path.
What do you mean by "we never call the expand_media_path"?
Do you expect we search the partition whether it has a default file,
then load the default file with efi_load_image() only when the
partition has a default file?
Thanks,
Masahisa Kojima
>
> So my suggestion would be
> - Drop #4
> - Adjust patch 5 and instead of loading the boot entry directly, scan
> for the special autogenerated boot option and look for that file there
>
> Heinrich would that work for you?
>
> Thanks
> /Ilias
> >
> > Thanks,
> > Masahisa Kojima
> >
> > > [0] https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html#load-option-processing
> > >
> > > Regards
> > > /Ilias
> > > >
> > > > Thanks,
> > > > Masahisa Kojima
> > > >
> > > > >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-19 6:45 ` Masahisa Kojima
@ 2023-10-19 7:22 ` Ilias Apalodimas
2023-10-19 8:24 ` Masahisa Kojima
0 siblings, 1 reply; 33+ messages in thread
From: Ilias Apalodimas @ 2023-10-19 7:22 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Heinrich Schuchardt, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
[...]
> > > > >
> > > > > According to the EDK II implementation[1], the boot option is
> > > > > enumerated with the following order.
> > > > > 1. Removable BlockIo
> > > > > 2. Fixed BlockIo
> > > > > 3. Non-BlockIo SimpleFileSystem
> > > > > 4. LoadFile
> > > > > So boot option for the fixed device such as HDD is also automatically created.
> > > > >
> > > > > [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L2150
> > > > >
> > > > > >
> > > > > > >
> > > > > > > When this boot option is selected, Fedora installer automatically starts.
> > > > > > > So EDK II is searching the default file on the fly.
> > > > > >
> > > > > > What is shown if you attach a medium without Bootaa64.efi?
> > > > >
> > > > > The same boot option is created.
> > > > > UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
> > > >
> > > > I went back to reading the spec and I think Heinrich is right. We
> > > > don't need that check at all. Going through [0] paragraph 4 says
> > > > " This search occurs when the device path of the boot image listed in
> > > > any boot option points directly to an EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> > > > device and does not specify the exact file to load"
> > > >
> > > > So we should *only* add an automatic variable without the default
> > > > application. Our code in try_load_entry() will search for that
> > > >
> > >
> > > Thank you for checking the UEFI specification and sorry for
> > > overlooking the above.
> > > So we will go back to the previous on the fly default application search.
> >
> > Yes, I was about to suggest that.
> > The problem as I understand it that the current patch not only
> > disregards disks and partitions that dont have a default (i.e
> > bootaa64.efi) file. It also *changes* the default boot option we add,
> > and instead of the disk it adds a file. That is the part that's
> > against the spec. On top of that it changes the behavior of efi
> > bootmgr and we never call the expand_media_path.
>
> What do you mean by "we never call the expand_media_path"?
> Do you expect we search the partition whether it has a default file,
> then load the default file with efi_load_image() only when the
> partition has a default file?
I mean, that the current patch also changed the behavior of the added
boot options.
Instead of adding a device path and using expand_media_path() in the
efibootmgr to load the image, we explicitly added the file path
instead
Regards
/Ilias
>
> Thanks,
> Masahisa Kojima
>
>
> >
> > So my suggestion would be
> > - Drop #4
> > - Adjust patch 5 and instead of loading the boot entry directly, scan
> > for the special autogenerated boot option and look for that file there
> >
> > Heinrich would that work for you?
> >
> > Thanks
> > /Ilias
> > >
> > > Thanks,
> > > Masahisa Kojima
> > >
> > > > [0] https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html#load-option-processing
> > > >
> > > > Regards
> > > > /Ilias
> > > > >
> > > > > Thanks,
> > > > > Masahisa Kojima
> > > > >
> > > > > >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-19 7:22 ` Ilias Apalodimas
@ 2023-10-19 8:24 ` Masahisa Kojima
0 siblings, 0 replies; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-19 8:24 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Heinrich Schuchardt, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
On Thu, 19 Oct 2023 at 16:22, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> [...]
>
> > > > > >
> > > > > > According to the EDK II implementation[1], the boot option is
> > > > > > enumerated with the following order.
> > > > > > 1. Removable BlockIo
> > > > > > 2. Fixed BlockIo
> > > > > > 3. Non-BlockIo SimpleFileSystem
> > > > > > 4. LoadFile
> > > > > > So boot option for the fixed device such as HDD is also automatically created.
> > > > > >
> > > > > > [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L2150
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > When this boot option is selected, Fedora installer automatically starts.
> > > > > > > > So EDK II is searching the default file on the fly.
> > > > > > >
> > > > > > > What is shown if you attach a medium without Bootaa64.efi?
> > > > > >
> > > > > > The same boot option is created.
> > > > > > UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
> > > > >
> > > > > I went back to reading the spec and I think Heinrich is right. We
> > > > > don't need that check at all. Going through [0] paragraph 4 says
> > > > > " This search occurs when the device path of the boot image listed in
> > > > > any boot option points directly to an EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
> > > > > device and does not specify the exact file to load"
> > > > >
> > > > > So we should *only* add an automatic variable without the default
> > > > > application. Our code in try_load_entry() will search for that
> > > > >
> > > >
> > > > Thank you for checking the UEFI specification and sorry for
> > > > overlooking the above.
> > > > So we will go back to the previous on the fly default application search.
> > >
> > > Yes, I was about to suggest that.
> > > The problem as I understand it that the current patch not only
> > > disregards disks and partitions that dont have a default (i.e
> > > bootaa64.efi) file. It also *changes* the default boot option we add,
> > > and instead of the disk it adds a file. That is the part that's
> > > against the spec. On top of that it changes the behavior of efi
> > > bootmgr and we never call the expand_media_path.
> >
> > What do you mean by "we never call the expand_media_path"?
> > Do you expect we search the partition whether it has a default file,
> > then load the default file with efi_load_image() only when the
> > partition has a default file?
>
> I mean, that the current patch also changed the behavior of the added
> boot options.
> Instead of adding a device path and using expand_media_path() in the
> efibootmgr to load the image, we explicitly added the file path
> instead
OK, I understand.
You explained what the current patch changed the efibootmgr behavior.
Thanks,
Masahisa Kojima
>
> Regards
> /Ilias
> >
> > Thanks,
> > Masahisa Kojima
> >
> >
> > >
> > > So my suggestion would be
> > > - Drop #4
> > > - Adjust patch 5 and instead of loading the boot entry directly, scan
> > > for the special autogenerated boot option and look for that file there
> > >
> > > Heinrich would that work for you?
> > >
> > > Thanks
> > > /Ilias
> > > >
> > > > Thanks,
> > > > Masahisa Kojima
> > > >
> > > > > [0] https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html#load-option-processing
> > > > >
> > > > > Regards
> > > > > /Ilias
> > > > > >
> > > > > > Thanks,
> > > > > > Masahisa Kojima
> > > > > >
> > > > > > >
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v7 4/9] efi_loader: create default file boot option
2023-10-17 5:32 ` Masahisa Kojima
2023-10-17 19:46 ` Ilias Apalodimas
@ 2023-10-17 20:29 ` Heinrich Schuchardt
1 sibling, 0 replies; 33+ messages in thread
From: Heinrich Schuchardt @ 2023-10-17 20:29 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Ilias Apalodimas, u-boot, Simon Glass, Takahiro Akashi,
Michal Simek
On 17.10.23 07:32, Masahisa Kojima wrote:
> Hi Heinrich,
>
> On Mon, 16 Oct 2023 at 23:52, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 16.10.23 15:00, Masahisa Kojima wrote:
>>> On Mon, 16 Oct 2023 at 21:46, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 16.10.23 14:31, Ilias Apalodimas wrote:
>>>>> Hi Heinrich,
>>>>>
>>>>> On Mon, 16 Oct 2023 at 10:06, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima <masahisa.kojima@linaro.org>:
>>>>>>> Current efibootmgr automatically creates the
>>>>>>> boot options of all disks and partitions installing
>>>>>>> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
>>>>>>> Some of the automatically created boot options are
>>>>>>> useless if the disk and partition does not have
>>>>>>> the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
>>>>>>>
>>>>>>> This commit only creates the boot option if the disk and
>>>>>>> partition have the default file so that system can directly
>>>>>>> boot from it.
>>>>>>
>>>>>> I don't directly see the user benefit.
>>>>>
>>>>> The user can add an HTTP boot option now and the installer will
>>>>> automatically start. That would allow products to ship with a single
>>>>> boot option provisioned and run an installer on first boot
>>>>
>>>> This commit is not about HTTP. It changes how boot options for block
>>>> devices are created.
>>>>
>>>>>
>>>>>>
>>>>>> Reading all file systems will increase the boot time. Shouldn't we avoid this?
>>>>>
>>>>> Any idea what would be an alternative? But when we added the
>>>>> automatic boot options we said we should avoid dynamic scanning and
>>>>> store results in a file. This is doing a similar thing. The only
>>>>> difference is that it mounts the iso image before adding the boot
>>>>> option.
>>>>
>>>> The alternative is to keep showing boot options for block devices even
>>>> if there is no BOOTxxxx.EFI file.
>>>>
>>>>>
>>>>>>
>>>>>> What does EDK II do?
>>>>>
>>>>> No Idea :)
>>>>
>>>>
>>>> On my workstation I get generated boot options
>>>>
>>>> Boot0001* UEFI:CD/DVD Drive BBS(129,,0x0)
>>>> Boot0003* UEFI:Removable Device BBS(130,,0x0)
>>>> Boot0004* UEFI:Network Device BBS(131,,0x0)
>>>>
>>>> without any media inserted and without any PXE server available.
>>>
>>> It is just information about how the EDK2 works.
>>> When I attach the Fedora installation media on EDK2(OVMF),
>>> the automatically generated boot option is as follows.
>>>
>>> UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
>>
>> An ATAPI drive typically is not removable. So I wonder why it is listed.
>> Did you set the removable flag on the command line?
>
> I guess it is not removable(actually I don't know how to set the
> device as removable).
> I just attached the iso image to QEMU with something like '-hda
> Fedora_netinst.iso".
>
> According to the EDK II implementation[1], the boot option is
> enumerated with the following order.
> 1. Removable BlockIo
> 2. Fixed BlockIo
> 3. Non-BlockIo SimpleFileSystem
> 4. LoadFile
> So boot option for the fixed device such as HDD is also automatically created.
>
> [1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c#L2150
>
>>
>>>
>>> When this boot option is selected, Fedora installer automatically starts.
>>> So EDK II is searching the default file on the fly.
>>
>> What is shown if you attach a medium without Bootaa64.efi?
>
> The same boot option is created.
> UEFI QEMU HARDDISK QM00001 : PciRoot(0x0)/Pci(0x1,0x1)/Ata(Primary,Master,0x0)
For USB devices there is a flag controlling if the device is removable:
qemu-system-riscv64 -M virt -kernel u-boot.bin -nographic -device
qemu-xhci -drive if=none,file=disk.img,format=raw,id=USB1 -device
usb-storage,drive=USB1,removable=on
This allows us to test the handling of removable devices in the boot
manager.
Best regards
Heinrich
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Thanks,
>>> Masahisa Kojima
>>>
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>> Thanks
>>>>> /Ilias
>>>>>>
>>>>>> Does the UEFI specification warrant this change?
>>>>>>
>>>>>> Best regards
>>>>>>
>>>>>> Heinrich
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>>>>>> ---
>>>>>>> lib/efi_loader/efi_bootmgr.c | 86 ++++++++++++++++++++++++++----------
>>>>>>> 1 file changed, 63 insertions(+), 23 deletions(-)
>>>>>>>
>>>>>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>>>>>> index a40762c74c..c8cf1c5506 100644
>>>>>>> --- a/lib/efi_loader/efi_bootmgr.c
>>>>>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>>>>>> @@ -355,40 +355,70 @@ 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;
>>>>>>> struct efi_load_option lo;
>>>>>>> char buf[BOOTMENU_DEVICE_NAME_MAX];
>>>>>>> - struct efi_device_path *device_path;
>>>>>>> + struct efi_device_path *device_path, *full_path, *dp, *fp;
>>>>>>> struct efi_device_path *short_dp;
>>>>>>> + struct efi_file_handle *root, *f;
>>>>>>> + struct efi_simple_file_system_protocol *file_system;
>>>>>>> + u16 *default_file_path = NULL;
>>>>>>>
>>>>>>> - ret = efi_search_protocol(volume_handles[i], &efi_guid_device_path, &handler);
>>>>>>> + ret = efi_search_protocol(volume_handles[i],
>>>>>>> + &efi_guid_device_path, &handler);
>>>>>>> if (ret != EFI_SUCCESS)
>>>>>>> continue;
>>>>>>> - ret = efi_protocol_open(handler, (void **)&device_path,
>>>>>>> - efi_root, NULL, EFI_OPEN_PROTOCOL_GET_PROTOCOL);
>>>>>>> +
>>>>>>> + device_path = handler->protocol_interface;
>>>>>>> + full_path = efi_dp_from_file(device_path,
>>>>>>> + "/EFI/BOOT/" BOOTEFI_NAME);
>>>>>>> +
>>>>>>> + /* check whether the partition or disk have the default file */
>>>>>>> + ret = efi_dp_split_file_path(full_path, &dp, &fp);
>>>>>>> + if (ret != EFI_SUCCESS || !fp)
>>>>>>> + goto next_entry;
>>>>>>> +
>>>>>>> + default_file_path = efi_dp_str(fp);
>>>>>>> + if (!default_file_path)
>>>>>>> + goto next_entry;
>>>>>>> +
>>>>>>> + ret = efi_search_protocol(volume_handles[i],
>>>>>>> + &efi_simple_file_system_protocol_guid,
>>>>>>> + &handler);
>>>>>>> if (ret != EFI_SUCCESS)
>>>>>>> - continue;
>>>>>>> + goto next_entry;
>>>>>>> +
>>>>>>> + file_system = handler->protocol_interface;
>>>>>>> + ret = EFI_CALL(file_system->open_volume(file_system, &root));
>>>>>>> + if (ret != EFI_SUCCESS)
>>>>>>> + goto next_entry;
>>>>>>> +
>>>>>>> + ret = EFI_CALL(root->open(root, &f, default_file_path,
>>>>>>> + EFI_FILE_MODE_READ, 0));
>>>>>>> + if (ret != EFI_SUCCESS)
>>>>>>> + goto next_entry;
>>>>>>> +
>>>>>>> + EFI_CALL(f->close(f));
>>>>>>>
>>>>>>> ret = efi_disk_get_device_name(volume_handles[i], buf, BOOTMENU_DEVICE_NAME_MAX);
>>>>>>> if (ret != EFI_SUCCESS)
>>>>>>> - continue;
>>>>>>> + goto next_entry;
>>>>>>>
>>>>>>> p = dev_name;
>>>>>>> utf8_utf16_strncpy(&p, buf, strlen(buf));
>>>>>>>
>>>>>>> /* prefer to short form device path */
>>>>>>> - short_dp = efi_dp_shorten(device_path);
>>>>>>> - if (short_dp)
>>>>>>> - device_path = short_dp;
>>>>>>> + short_dp = efi_dp_shorten(full_path);
>>>>>>> + device_path = short_dp ? short_dp : full_path;
>>>>>>>
>>>>>>> lo.label = dev_name;
>>>>>>> lo.attributes = LOAD_OPTION_ACTIVE;
>>>>>>> @@ -396,24 +426,35 @@ static efi_status_t efi_bootmgr_enumerate_boot_option(struct eficonfig_media_boo
>>>>>>> lo.file_path_length = efi_dp_size(device_path) + sizeof(END);
>>>>>>> /*
>>>>>>> * Set the dedicated guid to optional_data, it is used to identify
>>>>>>> - * the boot option that automatically generated by the bootmenu.
>>>>>>> + * the boot option that automatically generated by the efibootmgr.
>>>>>>> * efi_serialize_load_option() expects optional_data is null-terminated
>>>>>>> * utf8 string, so set the "1234567" string to allocate enough space
>>>>>>> * 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) {
>>>>>>> - ret = EFI_OUT_OF_RESOURCES;
>>>>>>> - goto out;
>>>>>>> + opt[num].size = efi_serialize_load_option(&lo, (u8 **)&opt[num].lo);
>>>>>>> + if (!opt[num].size) {
>>>>>>> + efi_free_pool(full_path);
>>>>>>> + efi_free_pool(dp);
>>>>>>> + efi_free_pool(fp);
>>>>>>> + efi_free_pool(default_file_path);
>>>>>>> + return EFI_OUT_OF_RESOURCES;
>>>>>>> }
>>>>>>> /* 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++;
>>>>>>> +
>>>>>>> +next_entry:
>>>>>>> + efi_free_pool(full_path);
>>>>>>> + efi_free_pool(dp);
>>>>>>> + efi_free_pool(fp);
>>>>>>> + efi_free_pool(default_file_path);
>>>>>>> }
>>>>>>>
>>>>>>> -out:
>>>>>>> - return ret;
>>>>>>> + *count = num;
>>>>>>> +
>>>>>>> + return EFI_SUCCESS;
>>>>>>> }
>>>>>>>
>>>>>>> /**
>>>>>>> @@ -642,7 +683,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.
>>>>>>> + * and create the boot option with default file if the file exists.
>>>>>>> * This function also provide the BOOT#### variable maintenance for
>>>>>>> * the media device entries.
>>>>>>> * - Automatically create the BOOT#### variable for the newly detected device,
>>>>>>> @@ -674,8 +715,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;
>>>>>>>
>>>>
>>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v7 5/9] efi_loader: support boot from URI device path
2023-10-16 6:45 [PATCH v7 0/9] Add EFI HTTP boot support Masahisa Kojima
` (3 preceding siblings ...)
2023-10-16 6:45 ` [PATCH v7 4/9] efi_loader: create default file boot option Masahisa Kojima
@ 2023-10-16 6:45 ` Masahisa Kojima
2023-10-18 0:42 ` Heinrich Schuchardt
2023-10-16 6:45 ` [PATCH v7 6/9] efi_loader: add CDROM short-form " Masahisa Kojima
` (3 subsequent siblings)
8 siblings, 1 reply; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-16 6:45 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, 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).
Since boot option indicating the default file is automatically
created when new disk is detected, system can boot by selecting
the automatically created blkmap boot option.
If the file is PE-COFF file, load and start the downloaded file.
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/Kconfig | 9 ++
lib/efi_loader/efi_bootmgr.c | 198 ++++++++++++++++++++++++++++++++++
lib/efi_loader/efi_dt_fixup.c | 2 +-
4 files changed, 210 insertions(+), 1 deletion(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index e24410505f..106006127b 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/Kconfig b/lib/efi_loader/Kconfig
index d20aaab6db..5d99206dc3 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -479,4 +479,13 @@ config EFI_RISCV_BOOT_PROTOCOL
replace the transfer via the device-tree. The latter is not
possible on systems using ACPI.
+config EFI_HTTP_BOOT
+ bool "EFI HTTP Boot support"
+ depends on CMD_DNS
+ depends on CMD_WGET
+ depends on BLKMAP
+ help
+ Enabling this option adds EFI HTTP Boot support. It allows to
+ directly boot from network.
+
endif
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index c8cf1c5506..c90b68f783 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,192 @@ 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;
+}
+
+/**
+ * load_mounted_image() - load mounted image with default file
+ *
+ * @devnum: target blkmap device
+ * @handle: pointer to handle for newly installed image
+ * Return: status code
+ */
+static efi_status_t load_mounted_image(int devnum, efi_handle_t *handle)
+{
+ u32 i;
+ u16 *bm_label, *p;
+ char device_name[12];
+ u16 *bootorder = NULL;
+ efi_uintn_t num, size;
+ void *load_option = NULL;
+ struct efi_load_option lo;
+ u16 varname[] = u"Boot####";
+ efi_status_t ret = EFI_NOT_FOUND;
+
+ snprintf(device_name, 12, "blkmap %d", devnum);
+ bm_label = calloc(1, (strlen(device_name) + 1) * sizeof(u16));
+ if (!bm_label)
+ return EFI_OUT_OF_RESOURCES;
+
+ p = bm_label;
+ utf8_utf16_strcpy(&p, device_name);
+
+ bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
+ if (!bootorder)
+ goto out;
+
+ num = size / sizeof(u16);
+ for (i = 0; i < num; i++) {
+ efi_create_indexed_name(varname, sizeof(varname), "Boot",
+ bootorder[i]);
+ load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
+ if (!load_option)
+ continue;
+
+ ret = efi_deserialize_load_option(&lo, load_option, &size);
+ if (ret != EFI_SUCCESS) {
+ free(load_option);
+ continue;
+ }
+
+ /* check whether the label indicates the target blkmap device */
+ if (u16_strncmp(bm_label, lo.label, u16_strlen(bm_label))) {
+ free(load_option);
+ continue;
+ }
+
+ /* check whether the boot option is automatically generated */
+ if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) {
+ free(load_option);
+ continue;
+ }
+
+ ret = EFI_CALL(efi_load_image(true, efi_root, lo.file_path,
+ NULL, 0, handle));
+ free(load_option);
+ goto out;
+ }
+
+ if (i == num)
+ ret = EFI_NOT_FOUND;
+out:
+ free(bm_label);
+ free(bootorder);
+
+ return ret;
+}
+
+/**
+ * try_load_from_uri_path() - Handle the URI device path
+ *
+ * @uridp: uri device path
+ * @lo_label: label of load option
+ * @handle: pointer to handle for newly installed image
+ * Return: status code
+ */
+static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
+ u16 *lo_label,
+ efi_handle_t *handle)
+{
+ char *s;
+ int err;
+ int uri_len;
+ u32 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);
+ err = wget_with_dns(image_addr, uridp->uri);
+ if (err < 0)
+ return EFI_INVALID_PARAMETER;
+ image_size = env_get_hex("filesize", 0);
+ if (!image_size)
+ 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);
+ if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
+ !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
+ struct udevice *blk;
+ struct blk_desc *desc;
+
+ blk = mount_image(lo_label, image_addr, image_size);
+ if (!blk)
+ return EFI_LOAD_ERROR;
+
+ /*
+ * When the new disk is detected, boot option is automatically
+ * created if it has a default file.
+ * Let's load the automatically created boot option.
+ */
+ desc = dev_get_uclass_plat(blk);
+ ret = load_mounted_image(desc->devnum, 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;
+
+ 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 ret;
+
+ 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_UNSUPPORTED;
+ }
+
+ return ret;
+}
+
/**
* try_load_entry() - try to load image for boot option
*
@@ -211,6 +401,14 @@ 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_EFI_HTTP_BOOT))
+ ret = try_load_from_uri_path(
+ (struct efi_device_path_uri *)
+ lo.file_path,
+ lo.label, handle);
+ else
+ ret = EFI_LOAD_ERROR;
} else {
file_path = expand_media_path(lo.file_path);
ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
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
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v7 5/9] efi_loader: support boot from URI device path
2023-10-16 6:45 ` [PATCH v7 5/9] efi_loader: support boot from URI device path Masahisa Kojima
@ 2023-10-18 0:42 ` Heinrich Schuchardt
2023-10-18 8:32 ` Masahisa Kojima
0 siblings, 1 reply; 33+ messages in thread
From: Heinrich Schuchardt @ 2023-10-18 0:42 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Ilias Apalodimas, Simon Glass, Takahiro Akashi, Michal Simek,
u-boot
On 16.10.23 08:45, 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).
> Since boot option indicating the default file is automatically
> created when new disk is detected, system can boot by selecting
> the automatically created blkmap boot option.
> If the file is PE-COFF file, load and start the downloaded file.
>
> 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/Kconfig | 9 ++
> lib/efi_loader/efi_bootmgr.c | 198 ++++++++++++++++++++++++++++++++++
> lib/efi_loader/efi_dt_fixup.c | 2 +-
> 4 files changed, 210 insertions(+), 1 deletion(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index e24410505f..106006127b 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/Kconfig b/lib/efi_loader/Kconfig
> index d20aaab6db..5d99206dc3 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -479,4 +479,13 @@ config EFI_RISCV_BOOT_PROTOCOL
> replace the transfer via the device-tree. The latter is not
> possible on systems using ACPI.
>
> +config EFI_HTTP_BOOT
> + bool "EFI HTTP Boot support"
> + depends on CMD_DNS
> + depends on CMD_WGET
> + depends on BLKMAP
> + help
> + Enabling this option adds EFI HTTP Boot support. It allows to
> + directly boot from network.
> +
> endif
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index c8cf1c5506..c90b68f783 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,192 @@ 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;
> +}
> +
> +/**
> + * load_mounted_image() - load mounted image with default file
> + *
> + * @devnum: target blkmap device
> + * @handle: pointer to handle for newly installed image
> + * Return: status code
> + */
> +static efi_status_t load_mounted_image(int devnum, efi_handle_t *handle)
> +{
> + u32 i;
> + u16 *bm_label, *p;
> + char device_name[12];
> + u16 *bootorder = NULL;
> + efi_uintn_t num, size;
> + void *load_option = NULL;
> + struct efi_load_option lo;
> + u16 varname[] = u"Boot####";
> + efi_status_t ret = EFI_NOT_FOUND;
> +
> + snprintf(device_name, 12, "blkmap %d", devnum);
> + bm_label = calloc(1, (strlen(device_name) + 1) * sizeof(u16));
> + if (!bm_label)
> + return EFI_OUT_OF_RESOURCES;
> +
> + p = bm_label;
> + utf8_utf16_strcpy(&p, device_name);
> +
> + bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> + if (!bootorder)
> + goto out;
> +
> + num = size / sizeof(u16);
> + for (i = 0; i < num; i++) {
> + efi_create_indexed_name(varname, sizeof(varname), "Boot",
> + bootorder[i]);
> + load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> + if (!load_option)
> + continue;
> +
> + ret = efi_deserialize_load_option(&lo, load_option, &size);
> + if (ret != EFI_SUCCESS) {
> + free(load_option);
> + continue;
> + }
> +
> + /* check whether the label indicates the target blkmap device */
> + if (u16_strncmp(bm_label, lo.label, u16_strlen(bm_label))) {
> + free(load_option);
> + continue;
> + }
> +
> + /* check whether the boot option is automatically generated */
> + if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) {
> + free(load_option);
> + continue;
> + }
> +
> + ret = EFI_CALL(efi_load_image(true, efi_root, lo.file_path,
> + NULL, 0, handle));
> + free(load_option);
> + goto out;
> + }
> +
> + if (i == num)
> + ret = EFI_NOT_FOUND;
> +out:
> + free(bm_label);
> + free(bootorder);
> +
> + return ret;
> +}
> +
> +/**
> + * try_load_from_uri_path() - Handle the URI device path
> + *
> + * @uridp: uri device path
> + * @lo_label: label of load option
> + * @handle: pointer to handle for newly installed image
> + * Return: status code
> + */
> +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> + u16 *lo_label,
> + efi_handle_t *handle)
> +{
> + char *s;
> + int err;
> + int uri_len;
> + u32 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);
> + err = wget_with_dns(image_addr, uridp->uri);
> + if (err < 0)
> + return EFI_INVALID_PARAMETER;
> + image_size = env_get_hex("filesize", 0);
> + if (!image_size)
> + 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);
> + if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
> + !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
> + struct udevice *blk;
> + struct blk_desc *desc;
> +
> + blk = mount_image(lo_label, image_addr, image_size);
> + if (!blk)
> + return EFI_LOAD_ERROR;
> +
> + /*
> + * When the new disk is detected, boot option is automatically
> + * created if it has a default file.
> + * Let's load the automatically created boot option.
> + */
> + desc = dev_get_uclass_plat(blk);
> + ret = load_mounted_image(desc->devnum, handle);
> + if (ret != EFI_SUCCESS)
> + return ret;
> +
> + /* whole ramdisk must be reserved */
> + efi_reserve_memory(image_addr, image_size, true);
This comment is not enough to explain why you make the reservation.
Our blkmap driver is gone after ExitBootServices().
Why is EFI_LOADER_DATA not good enough?
How will the image be passed to Linux?
Best regards
Heinrich
> + } 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;
> +
> + 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 ret;
> +
> + 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_UNSUPPORTED;
> + }
> +
> + return ret;
> +}
> +
> /**
> * try_load_entry() - try to load image for boot option
> *
> @@ -211,6 +401,14 @@ 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_EFI_HTTP_BOOT))
> + ret = try_load_from_uri_path(
> + (struct efi_device_path_uri *)
> + lo.file_path,
> + lo.label, handle);
> + else
> + ret = EFI_LOAD_ERROR;
> } else {
> file_path = expand_media_path(lo.file_path);
> ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> 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;
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v7 5/9] efi_loader: support boot from URI device path
2023-10-18 0:42 ` Heinrich Schuchardt
@ 2023-10-18 8:32 ` Masahisa Kojima
0 siblings, 0 replies; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-18 8:32 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Ilias Apalodimas, Simon Glass, Takahiro Akashi, Michal Simek,
u-boot
Hi Heinrich,
On Wed, 18 Oct 2023 at 09:42, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 16.10.23 08:45, 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).
> > Since boot option indicating the default file is automatically
> > created when new disk is detected, system can boot by selecting
> > the automatically created blkmap boot option.
> > If the file is PE-COFF file, load and start the downloaded file.
> >
> > 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/Kconfig | 9 ++
> > lib/efi_loader/efi_bootmgr.c | 198 ++++++++++++++++++++++++++++++++++
> > lib/efi_loader/efi_dt_fixup.c | 2 +-
> > 4 files changed, 210 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index e24410505f..106006127b 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/Kconfig b/lib/efi_loader/Kconfig
> > index d20aaab6db..5d99206dc3 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -479,4 +479,13 @@ config EFI_RISCV_BOOT_PROTOCOL
> > replace the transfer via the device-tree. The latter is not
> > possible on systems using ACPI.
> >
> > +config EFI_HTTP_BOOT
> > + bool "EFI HTTP Boot support"
> > + depends on CMD_DNS
> > + depends on CMD_WGET
> > + depends on BLKMAP
> > + help
> > + Enabling this option adds EFI HTTP Boot support. It allows to
> > + directly boot from network.
> > +
> > endif
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index c8cf1c5506..c90b68f783 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,192 @@ 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;
> > +}
> > +
> > +/**
> > + * load_mounted_image() - load mounted image with default file
> > + *
> > + * @devnum: target blkmap device
> > + * @handle: pointer to handle for newly installed image
> > + * Return: status code
> > + */
> > +static efi_status_t load_mounted_image(int devnum, efi_handle_t *handle)
> > +{
> > + u32 i;
> > + u16 *bm_label, *p;
> > + char device_name[12];
> > + u16 *bootorder = NULL;
> > + efi_uintn_t num, size;
> > + void *load_option = NULL;
> > + struct efi_load_option lo;
> > + u16 varname[] = u"Boot####";
> > + efi_status_t ret = EFI_NOT_FOUND;
> > +
> > + snprintf(device_name, 12, "blkmap %d", devnum);
> > + bm_label = calloc(1, (strlen(device_name) + 1) * sizeof(u16));
> > + if (!bm_label)
> > + return EFI_OUT_OF_RESOURCES;
> > +
> > + p = bm_label;
> > + utf8_utf16_strcpy(&p, device_name);
> > +
> > + bootorder = efi_get_var(u"BootOrder", &efi_global_variable_guid, &size);
> > + if (!bootorder)
> > + goto out;
> > +
> > + num = size / sizeof(u16);
> > + for (i = 0; i < num; i++) {
> > + efi_create_indexed_name(varname, sizeof(varname), "Boot",
> > + bootorder[i]);
> > + load_option = efi_get_var(varname, &efi_global_variable_guid, &size);
> > + if (!load_option)
> > + continue;
> > +
> > + ret = efi_deserialize_load_option(&lo, load_option, &size);
> > + if (ret != EFI_SUCCESS) {
> > + free(load_option);
> > + continue;
> > + }
> > +
> > + /* check whether the label indicates the target blkmap device */
> > + if (u16_strncmp(bm_label, lo.label, u16_strlen(bm_label))) {
> > + free(load_option);
> > + continue;
> > + }
> > +
> > + /* check whether the boot option is automatically generated */
> > + if (guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) {
> > + free(load_option);
> > + continue;
> > + }
> > +
> > + ret = EFI_CALL(efi_load_image(true, efi_root, lo.file_path,
> > + NULL, 0, handle));
> > + free(load_option);
> > + goto out;
> > + }
> > +
> > + if (i == num)
> > + ret = EFI_NOT_FOUND;
> > +out:
> > + free(bm_label);
> > + free(bootorder);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * try_load_from_uri_path() - Handle the URI device path
> > + *
> > + * @uridp: uri device path
> > + * @lo_label: label of load option
> > + * @handle: pointer to handle for newly installed image
> > + * Return: status code
> > + */
> > +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> > + u16 *lo_label,
> > + efi_handle_t *handle)
> > +{
> > + char *s;
> > + int err;
> > + int uri_len;
> > + u32 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);
> > + err = wget_with_dns(image_addr, uridp->uri);
> > + if (err < 0)
> > + return EFI_INVALID_PARAMETER;
> > + image_size = env_get_hex("filesize", 0);
> > + if (!image_size)
> > + 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);
> > + if (!strncmp(&uridp->uri[uri_len - 4], ".iso", 4) ||
> > + !strncmp(&uridp->uri[uri_len - 4], ".img", 4)) {
> > + struct udevice *blk;
> > + struct blk_desc *desc;
> > +
> > + blk = mount_image(lo_label, image_addr, image_size);
> > + if (!blk)
> > + return EFI_LOAD_ERROR;
> > +
> > + /*
> > + * When the new disk is detected, boot option is automatically
> > + * created if it has a default file.
> > + * Let's load the automatically created boot option.
> > + */
> > + desc = dev_get_uclass_plat(blk);
> > + ret = load_mounted_image(desc->devnum, handle);
> > + if (ret != EFI_SUCCESS)
> > + return ret;
> > +
> > + /* whole ramdisk must be reserved */
> > + efi_reserve_memory(image_addr, image_size, true);
>
> This comment is not enough to explain why you make the reservation.
>
> Our blkmap driver is gone after ExitBootServices().
> Why is EFI_LOADER_DATA not good enough?
In current implementation, this ramdisk is not passed to Linux,
so this whole ramdisk is not required to be reserved.
> How will the image be passed to Linux?
If the kernel supports 'pmem'(drivers/nvdimm/of_pmem.c), U-Boot can
expose the ramdisk.
For example, with the following 'pmem' device-tree node,
linux can mount the 256MB ramdisk starting from 0x90000000.
+ pmem@90000000 {
+ compatible = "pmem-region";
+ reg = < 0x0 0x90000000 0x0 0x10000000 >;
+ volatile;
+ };
In my check, SUSE installer supports this pmem feature.
I will try to integrate this in this series.
Thanks,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
>
> > + } 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;
> > +
> > + 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 ret;
> > +
> > + 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_UNSUPPORTED;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > /**
> > * try_load_entry() - try to load image for boot option
> > *
> > @@ -211,6 +401,14 @@ 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_EFI_HTTP_BOOT))
> > + ret = try_load_from_uri_path(
> > + (struct efi_device_path_uri *)
> > + lo.file_path,
> > + lo.label, handle);
> > + else
> > + ret = EFI_LOAD_ERROR;
> > } else {
> > file_path = expand_media_path(lo.file_path);
> > ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> > 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;
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v7 6/9] efi_loader: add CDROM short-form device path
2023-10-16 6:45 [PATCH v7 0/9] Add EFI HTTP boot support Masahisa Kojima
` (4 preceding siblings ...)
2023-10-16 6:45 ` [PATCH v7 5/9] efi_loader: support boot from URI device path Masahisa Kojima
@ 2023-10-16 6:45 ` Masahisa Kojima
2023-10-18 10:19 ` Heinrich Schuchardt
2023-10-16 6:45 ` [PATCH v7 7/9] Boot var automatic management for removable medias Masahisa Kojima
` (2 subsequent siblings)
8 siblings, 1 reply; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-16 6:45 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, Masahisa Kojima
UEFI specification does not mandate to support the short-form
of the CDROM media device path.
Fedora installation ISO image is identified as CDROM media
device path, supporting short-form CDROM media device path is
required to automatically add the boot option having default
file of Fedora installation image.
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
lib/efi_loader/efi_device_path.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
index ed7214f3a3..ac673ab117 100644
--- a/lib/efi_loader/efi_device_path.c
+++ b/lib/efi_loader/efi_device_path.c
@@ -110,7 +110,8 @@ struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp)
while (dp) {
if (EFI_DP_TYPE(dp, MESSAGING_DEVICE, MSG_USB_WWI) ||
EFI_DP_TYPE(dp, MEDIA_DEVICE, HARD_DRIVE_PATH) ||
- EFI_DP_TYPE(dp, MEDIA_DEVICE, FILE_PATH))
+ EFI_DP_TYPE(dp, MEDIA_DEVICE, FILE_PATH) ||
+ EFI_DP_TYPE(dp, MEDIA_DEVICE, CDROM_PATH))
return dp;
dp = efi_dp_next(dp);
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v7 6/9] efi_loader: add CDROM short-form device path
2023-10-16 6:45 ` [PATCH v7 6/9] efi_loader: add CDROM short-form " Masahisa Kojima
@ 2023-10-18 10:19 ` Heinrich Schuchardt
2023-10-19 3:21 ` Masahisa Kojima
0 siblings, 1 reply; 33+ messages in thread
From: Heinrich Schuchardt @ 2023-10-18 10:19 UTC (permalink / raw)
To: Masahisa Kojima
Cc: Ilias Apalodimas, Simon Glass, Takahiro Akashi, Michal Simek,
u-boot
On 10/16/23 08:45, Masahisa Kojima wrote:
> UEFI specification does not mandate to support the short-form
> of the CDROM media device path.
> Fedora installation ISO image is identified as CDROM media
> device path, supporting short-form CDROM media device path is
> required to automatically add the boot option having default
> file of Fedora installation image.
How is the CDROM media path created?
Why would the image not be found if the path is not shortened?
What is Fedora specific here?
What does EDK II do?
Best regards
Heinrich
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> lib/efi_loader/efi_device_path.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> index ed7214f3a3..ac673ab117 100644
> --- a/lib/efi_loader/efi_device_path.c
> +++ b/lib/efi_loader/efi_device_path.c
> @@ -110,7 +110,8 @@ struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp)
> while (dp) {
> if (EFI_DP_TYPE(dp, MESSAGING_DEVICE, MSG_USB_WWI) ||
> EFI_DP_TYPE(dp, MEDIA_DEVICE, HARD_DRIVE_PATH) ||
> - EFI_DP_TYPE(dp, MEDIA_DEVICE, FILE_PATH))
> + EFI_DP_TYPE(dp, MEDIA_DEVICE, FILE_PATH) ||
> + EFI_DP_TYPE(dp, MEDIA_DEVICE, CDROM_PATH))
> return dp;
>
> dp = efi_dp_next(dp);
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v7 6/9] efi_loader: add CDROM short-form device path
2023-10-18 10:19 ` Heinrich Schuchardt
@ 2023-10-19 3:21 ` Masahisa Kojima
0 siblings, 0 replies; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-19 3:21 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Ilias Apalodimas, Simon Glass, Takahiro Akashi, Michal Simek,
u-boot
Hi Heinrich,
On Wed, 18 Oct 2023 at 19:25, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 10/16/23 08:45, Masahisa Kojima wrote:
> > UEFI specification does not mandate to support the short-form
> > of the CDROM media device path.
> > Fedora installation ISO image is identified as CDROM media
> > device path, supporting short-form CDROM media device path is
> > required to automatically add the boot option having default
> > file of Fedora installation image.
>
> How is the CDROM media path created?
Fedora installation media only has a ISO 9660 filesystem without
MBR/GPT partition table.
U-Boot disk driver classifies it as PART_TYPE_ISO, then EFI-subsystem
creates a CDROM media path for it.
> Why would the image not be found if the path is not shortened?
We discussed in the separate patch that we drop the patch#4(automatically
create the boot option with default application file path appended),
so this patch is also dropped.
> What is Fedora specific here?
Fedora installation media has ISO 9660 filesystem only.
Other disto such as Debian has a MBR partition table, U-Boot creates
HardDisk device path for these medias.
> What does EDK II do?
EDK II does the same for Fedora install image, CDROM media path is created.
Thanks,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > lib/efi_loader/efi_device_path.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/efi_loader/efi_device_path.c b/lib/efi_loader/efi_device_path.c
> > index ed7214f3a3..ac673ab117 100644
> > --- a/lib/efi_loader/efi_device_path.c
> > +++ b/lib/efi_loader/efi_device_path.c
> > @@ -110,7 +110,8 @@ struct efi_device_path *efi_dp_shorten(struct efi_device_path *dp)
> > while (dp) {
> > if (EFI_DP_TYPE(dp, MESSAGING_DEVICE, MSG_USB_WWI) ||
> > EFI_DP_TYPE(dp, MEDIA_DEVICE, HARD_DRIVE_PATH) ||
> > - EFI_DP_TYPE(dp, MEDIA_DEVICE, FILE_PATH))
> > + EFI_DP_TYPE(dp, MEDIA_DEVICE, FILE_PATH) ||
> > + EFI_DP_TYPE(dp, MEDIA_DEVICE, CDROM_PATH))
> > return dp;
> >
> > dp = efi_dp_next(dp);
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v7 7/9] Boot var automatic management for removable medias
2023-10-16 6:45 [PATCH v7 0/9] Add EFI HTTP boot support Masahisa Kojima
` (5 preceding siblings ...)
2023-10-16 6:45 ` [PATCH v7 6/9] efi_loader: add CDROM short-form " Masahisa Kojima
@ 2023-10-16 6:45 ` Masahisa Kojima
2023-10-16 7:58 ` João Marcos Costa
2023-10-16 6:45 ` [PATCH v7 8/9] cmd: efidebug: add uri device path Masahisa Kojima
2023-10-16 6:45 ` [PATCH v7 9/9] doc: uefi: add HTTP Boot support Masahisa Kojima
8 siblings, 1 reply; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-16 6:45 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, Raymond Mao, Masahisa Kojima,
Joao Marcos Costa, Thomas Petazzoni, Miquel Raynal
From: Raymond Mao <raymond.mao@linaro.org>
Changes for complying to EFI spec §3.5.1.1
'Removable Media Boot Behavior'.
Boot variables can be automatically generated during a removable
media is probed. At the same time, unused boot variables will be
detected and removed.
Please note that currently the function 'efi_disk_remove' has no
ability to distinguish below two scenarios
a) Unplugging of a removable media under U-Boot
b) U-Boot exiting and booting an OS
Thus currently the boot variables management is not added into
'efi_disk_remove' to avoid boot options being added/erased
repeatedly under scenario b) during power cycles
See TODO comments under function 'efi_disk_remove' for more details
The original efi_secboot tests expect that BootOrder EFI variable
is not defined. With this commit, the BootOrder EFI variable is
automatically added when the disk is detected. The original
efi_secboot tests end up with unexpected failure.
The efi_secboot tests need to be modified to explicitly set
the BootOrder EFI variable.
squashfs ls test is also affected by this modification, need to
clear the previous state before squashfs ls test starts.
Co-developed-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
lib/efi_loader/efi_disk.c | 18 ++++++++
lib/efi_loader/efi_setup.c | 7 ++++
test/py/tests/test_efi_secboot/test_signed.py | 42 +++++++++----------
.../test_efi_secboot/test_signed_intca.py | 14 +++----
.../tests/test_efi_secboot/test_unsigned.py | 14 +++----
.../test_fs/test_squashfs/test_sqfs_ls.py | 6 +++
6 files changed, 66 insertions(+), 35 deletions(-)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index f0d76113b0..b808a7fe62 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -690,6 +690,13 @@ int efi_disk_probe(void *ctx, struct event *event)
return -1;
}
+ /* only do the boot option management when UEFI sub-system is initialized */
+ if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR) && efi_obj_list_initialized == EFI_SUCCESS) {
+ ret = efi_bootmgr_update_media_device_boot_option();
+ if (ret != EFI_SUCCESS)
+ return -1;
+ }
+
return 0;
}
@@ -742,6 +749,17 @@ int efi_disk_remove(void *ctx, struct event *event)
dev_tag_del(dev, DM_TAG_EFI);
return 0;
+
+ /*
+ * TODO A flag to distinguish below 2 different scenarios of this
+ * function call is needed:
+ * a) Unplugging of a removable media under U-Boot
+ * b) U-Boot exiting and booting an OS
+ * In case of scenario a), efi_bootmgr_update_media_device_boot_option()
+ * needs to be invoked here to update the boot options and remove the
+ * unnecessary ones.
+ */
+
}
/**
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index e6de685e87..37359a77bb 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -245,6 +245,13 @@ efi_status_t efi_init_obj_list(void)
if (ret != EFI_SUCCESS)
goto out;
+ if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
+ /* update boot option after variable service initialized */
+ ret = efi_bootmgr_update_media_device_boot_option();
+ if (ret != EFI_SUCCESS)
+ goto out;
+ }
+
/* Define supported languages */
ret = efi_init_platform_lang();
if (ret != EFI_SUCCESS)
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index ca52e853d8..2f862a259a 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -29,7 +29,7 @@ class TestEfiSignedImage(object):
output = u_boot_console.run_command_list([
'host bind 0 %s' % disk_img,
'efidebug boot add -b 1 HELLO1 host 0:1 /helloworld.efi.signed -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'bootefi bootmgr'])
assert 'Hello, world!' in ''.join(output)
@@ -37,7 +37,7 @@ class TestEfiSignedImage(object):
# Test Case 1b, run unsigned image if no PK
output = u_boot_console.run_command_list([
'efidebug boot add -b 2 HELLO2 host 0:1 /helloworld.efi -s ""',
- 'efidebug boot next 2',
+ 'efidebug boot order 2',
'bootefi bootmgr'])
assert 'Hello, world!' in ''.join(output)
@@ -59,13 +59,13 @@ class TestEfiSignedImage(object):
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO1 host 0:1 /helloworld.efi.signed -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert('\'HELLO1\' failed' in ''.join(output))
assert('efi_start_image() returned: 26' in ''.join(output))
output = u_boot_console.run_command_list([
'efidebug boot add -b 2 HELLO2 host 0:1 /helloworld.efi -s ""',
- 'efidebug boot next 2',
+ 'efidebug boot order 2',
'efidebug test bootmgr'])
assert '\'HELLO2\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -77,12 +77,12 @@ class TestEfiSignedImage(object):
'setenv -e -nv -bs -rt -at -i 4000000:$filesize db'])
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 2',
+ 'efidebug boot order 2',
'efidebug test bootmgr'])
assert '\'HELLO2\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'bootefi bootmgr'])
assert 'Hello, world!' in ''.join(output)
@@ -105,7 +105,7 @@ class TestEfiSignedImage(object):
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -117,7 +117,7 @@ class TestEfiSignedImage(object):
'setenv -e -nv -bs -rt -at -i 4000000:$filesize db'])
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -143,7 +143,7 @@ class TestEfiSignedImage(object):
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -170,7 +170,7 @@ class TestEfiSignedImage(object):
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
@@ -181,7 +181,7 @@ class TestEfiSignedImage(object):
'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db'])
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
@@ -193,7 +193,7 @@ class TestEfiSignedImage(object):
'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -205,7 +205,7 @@ class TestEfiSignedImage(object):
'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize dbx'])
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -230,7 +230,7 @@ class TestEfiSignedImage(object):
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -254,7 +254,7 @@ class TestEfiSignedImage(object):
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'bootefi bootmgr'])
assert 'Hello, world!' in ''.join(output)
@@ -265,7 +265,7 @@ class TestEfiSignedImage(object):
'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -279,7 +279,7 @@ class TestEfiSignedImage(object):
'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -307,7 +307,7 @@ class TestEfiSignedImage(object):
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -330,7 +330,7 @@ class TestEfiSignedImage(object):
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -349,7 +349,7 @@ class TestEfiSignedImage(object):
output = u_boot_console.run_command_list([
'host bind 0 %s' % disk_img,
'efidebug boot add -b 1 HELLO1 host 0:1 /helloworld_forged.efi.signed -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert('hELLO, world!' in ''.join(output))
@@ -364,7 +364,7 @@ class TestEfiSignedImage(object):
'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK'])
assert 'Failed to set EFI variable' not in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert(not 'hELLO, world!' in ''.join(output))
assert('\'HELLO1\' failed' in ''.join(output))
diff --git a/test/py/tests/test_efi_secboot/test_signed_intca.py b/test/py/tests/test_efi_secboot/test_signed_intca.py
index d8d599d22f..8d9a5f3e7f 100644
--- a/test/py/tests/test_efi_secboot/test_signed_intca.py
+++ b/test/py/tests/test_efi_secboot/test_signed_intca.py
@@ -40,7 +40,7 @@ class TestEfiSignedImageIntca(object):
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO_a host 0:1 /helloworld.efi.signed_a -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO_a\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -49,7 +49,7 @@ class TestEfiSignedImageIntca(object):
# Test Case 1b, signed and authenticated by root CA
output = u_boot_console.run_command_list([
'efidebug boot add -b 2 HELLO_ab host 0:1 /helloworld.efi.signed_ab -s ""',
- 'efidebug boot next 2',
+ 'efidebug boot order 2',
'bootefi bootmgr'])
assert 'Hello, world!' in ''.join(output)
@@ -71,7 +71,7 @@ class TestEfiSignedImageIntca(object):
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO_abc host 0:1 /helloworld.efi.signed_abc -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO_abc\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -81,7 +81,7 @@ class TestEfiSignedImageIntca(object):
output = u_boot_console.run_command_list([
'fatload host 0:1 4000000 db_b.auth',
'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO_abc\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
@@ -91,7 +91,7 @@ class TestEfiSignedImageIntca(object):
output = u_boot_console.run_command_list([
'fatload host 0:1 4000000 db_c.auth',
'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
@@ -117,7 +117,7 @@ class TestEfiSignedImageIntca(object):
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO_abc host 0:1 /helloworld.efi.signed_abc -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert 'Hello, world!' in ''.join(output)
# Or,
@@ -129,7 +129,7 @@ class TestEfiSignedImageIntca(object):
output = u_boot_console.run_command_list([
'fatload host 0:1 4000000 dbx_c.auth',
'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert '\'HELLO_abc\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
diff --git a/test/py/tests/test_efi_secboot/test_unsigned.py b/test/py/tests/test_efi_secboot/test_unsigned.py
index df63f0df08..7c078f220d 100644
--- a/test/py/tests/test_efi_secboot/test_unsigned.py
+++ b/test/py/tests/test_efi_secboot/test_unsigned.py
@@ -36,11 +36,11 @@ class TestEfiUnsignedImage(object):
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'bootefi bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert 'efi_start_image() returned: 26' in ''.join(output)
assert 'Hello, world!' not in ''.join(output)
@@ -65,7 +65,7 @@ class TestEfiUnsignedImage(object):
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'bootefi bootmgr'])
assert 'Hello, world!' in ''.join(output)
@@ -89,11 +89,11 @@ class TestEfiUnsignedImage(object):
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'bootefi bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert 'efi_start_image() returned: 26' in ''.join(output)
assert 'Hello, world!' not in ''.join(output)
@@ -107,11 +107,11 @@ class TestEfiUnsignedImage(object):
output = u_boot_console.run_command_list([
'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi -s ""',
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'bootefi bootmgr'])
assert '\'HELLO\' failed' in ''.join(output)
output = u_boot_console.run_command_list([
- 'efidebug boot next 1',
+ 'efidebug boot order 1',
'efidebug test bootmgr'])
assert 'efi_start_image() returned: 26' in ''.join(output)
assert 'Hello, world!' not in ''.join(output)
diff --git a/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py b/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py
index 527a556ed8..3b8118104f 100644
--- a/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py
+++ b/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py
@@ -118,6 +118,12 @@ def test_sqfs_ls(u_boot_console):
"""
build_dir = u_boot_console.config.build_dir
+ # If the EFI subsystem is enabled, default file(e.g. EFI/BOOT/BOOTAA64.EFI)
+ # is scanned when the new disk is detected. This ends up with the unexpected
+ # output at the first 'sqfsls' command.
+ # Clear the previous state.
+ u_boot_console.restart_uboot()
+
# setup test environment
check_mksquashfs_version()
generate_sqfs_src_dir(build_dir)
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v7 7/9] Boot var automatic management for removable medias
2023-10-16 6:45 ` [PATCH v7 7/9] Boot var automatic management for removable medias Masahisa Kojima
@ 2023-10-16 7:58 ` João Marcos Costa
2023-10-16 19:02 ` João Marcos Costa
0 siblings, 1 reply; 33+ messages in thread
From: João Marcos Costa @ 2023-10-16 7:58 UTC (permalink / raw)
To: Masahisa Kojima
Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, Raymond Mao, Thomas Petazzoni,
Miquel Raynal
Hello,
Em seg., 16 de out. de 2023 às 08:47, Masahisa Kojima <
masahisa.kojima@linaro.org> escreveu:
> From: Raymond Mao <raymond.mao@linaro.org>
>
> Changes for complying to EFI spec §3.5.1.1
> 'Removable Media Boot Behavior'.
> Boot variables can be automatically generated during a removable
> media is probed. At the same time, unused boot variables will be
> detected and removed.
>
> Please note that currently the function 'efi_disk_remove' has no
> ability to distinguish below two scenarios
> a) Unplugging of a removable media under U-Boot
> b) U-Boot exiting and booting an OS
> Thus currently the boot variables management is not added into
> 'efi_disk_remove' to avoid boot options being added/erased
> repeatedly under scenario b) during power cycles
> See TODO comments under function 'efi_disk_remove' for more details
>
> The original efi_secboot tests expect that BootOrder EFI variable
> is not defined. With this commit, the BootOrder EFI variable is
> automatically added when the disk is detected. The original
> efi_secboot tests end up with unexpected failure.
> The efi_secboot tests need to be modified to explicitly set
> the BootOrder EFI variable.
>
> squashfs ls test is also affected by this modification, need to
> clear the previous state before squashfs ls test starts.
>
> Co-developed-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> lib/efi_loader/efi_disk.c | 18 ++++++++
> lib/efi_loader/efi_setup.c | 7 ++++
> test/py/tests/test_efi_secboot/test_signed.py | 42 +++++++++----------
> .../test_efi_secboot/test_signed_intca.py | 14 +++----
> .../tests/test_efi_secboot/test_unsigned.py | 14 +++----
> .../test_fs/test_squashfs/test_sqfs_ls.py | 6 +++
> 6 files changed, 66 insertions(+), 35 deletions(-)
>
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index f0d76113b0..b808a7fe62 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -690,6 +690,13 @@ int efi_disk_probe(void *ctx, struct event *event)
> return -1;
> }
>
> + /* only do the boot option management when UEFI sub-system is
> initialized */
> + if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR) &&
> efi_obj_list_initialized == EFI_SUCCESS) {
> + ret = efi_bootmgr_update_media_device_boot_option();
> + if (ret != EFI_SUCCESS)
> + return -1;
> + }
> +
> return 0;
> }
>
> @@ -742,6 +749,17 @@ int efi_disk_remove(void *ctx, struct event *event)
> dev_tag_del(dev, DM_TAG_EFI);
>
> return 0;
> +
> + /*
> + * TODO A flag to distinguish below 2 different scenarios of this
> + * function call is needed:
> + * a) Unplugging of a removable media under U-Boot
> + * b) U-Boot exiting and booting an OS
> + * In case of scenario a),
> efi_bootmgr_update_media_device_boot_option()
> + * needs to be invoked here to update the boot options and remove
> the
> + * unnecessary ones.
> + */
> +
> }
>
> /**
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index e6de685e87..37359a77bb 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -245,6 +245,13 @@ efi_status_t efi_init_obj_list(void)
> if (ret != EFI_SUCCESS)
> goto out;
>
> + if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) {
> + /* update boot option after variable service initialized */
> + ret = efi_bootmgr_update_media_device_boot_option();
> + if (ret != EFI_SUCCESS)
> + goto out;
> + }
> +
> /* Define supported languages */
> ret = efi_init_platform_lang();
> if (ret != EFI_SUCCESS)
> diff --git a/test/py/tests/test_efi_secboot/test_signed.py
> b/test/py/tests/test_efi_secboot/test_signed.py
> index ca52e853d8..2f862a259a 100644
> --- a/test/py/tests/test_efi_secboot/test_signed.py
> +++ b/test/py/tests/test_efi_secboot/test_signed.py
> @@ -29,7 +29,7 @@ class TestEfiSignedImage(object):
> output = u_boot_console.run_command_list([
> 'host bind 0 %s' % disk_img,
> 'efidebug boot add -b 1 HELLO1 host 0:1
> /helloworld.efi.signed -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'bootefi bootmgr'])
> assert 'Hello, world!' in ''.join(output)
>
> @@ -37,7 +37,7 @@ class TestEfiSignedImage(object):
> # Test Case 1b, run unsigned image if no PK
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 2 HELLO2 host 0:1 /helloworld.efi
> -s ""',
> - 'efidebug boot next 2',
> + 'efidebug boot order 2',
> 'bootefi bootmgr'])
> assert 'Hello, world!' in ''.join(output)
>
> @@ -59,13 +59,13 @@ class TestEfiSignedImage(object):
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO1 host 0:1
> /helloworld.efi.signed -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert('\'HELLO1\' failed' in ''.join(output))
> assert('efi_start_image() returned: 26' in ''.join(output))
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 2 HELLO2 host 0:1 /helloworld.efi
> -s ""',
> - 'efidebug boot next 2',
> + 'efidebug boot order 2',
> 'efidebug test bootmgr'])
> assert '\'HELLO2\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -77,12 +77,12 @@ class TestEfiSignedImage(object):
> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db'])
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 2',
> + 'efidebug boot order 2',
> 'efidebug test bootmgr'])
> assert '\'HELLO2\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'bootefi bootmgr'])
> assert 'Hello, world!' in ''.join(output)
>
> @@ -105,7 +105,7 @@ class TestEfiSignedImage(object):
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1
> /helloworld.efi.signed -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -117,7 +117,7 @@ class TestEfiSignedImage(object):
> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db'])
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -143,7 +143,7 @@ class TestEfiSignedImage(object):
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1
> /helloworld.efi.signed -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -170,7 +170,7 @@ class TestEfiSignedImage(object):
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1
> /helloworld.efi.signed_2sigs -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert 'Hello, world!' in ''.join(output)
>
> @@ -181,7 +181,7 @@ class TestEfiSignedImage(object):
> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db'])
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert 'Hello, world!' in ''.join(output)
>
> @@ -193,7 +193,7 @@ class TestEfiSignedImage(object):
> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -205,7 +205,7 @@ class TestEfiSignedImage(object):
> 'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize dbx'])
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -230,7 +230,7 @@ class TestEfiSignedImage(object):
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1
> /helloworld.efi.signed_2sigs -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -254,7 +254,7 @@ class TestEfiSignedImage(object):
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1
> /helloworld.efi.signed -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'bootefi bootmgr'])
> assert 'Hello, world!' in ''.join(output)
>
> @@ -265,7 +265,7 @@ class TestEfiSignedImage(object):
> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -279,7 +279,7 @@ class TestEfiSignedImage(object):
> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -307,7 +307,7 @@ class TestEfiSignedImage(object):
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1
> /helloworld.efi.signed_2sigs -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -330,7 +330,7 @@ class TestEfiSignedImage(object):
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1
> /helloworld.efi.signed_2sigs -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -349,7 +349,7 @@ class TestEfiSignedImage(object):
> output = u_boot_console.run_command_list([
> 'host bind 0 %s' % disk_img,
> 'efidebug boot add -b 1 HELLO1 host 0:1
> /helloworld_forged.efi.signed -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert('hELLO, world!' in ''.join(output))
>
> @@ -364,7 +364,7 @@ class TestEfiSignedImage(object):
> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK'])
> assert 'Failed to set EFI variable' not in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert(not 'hELLO, world!' in ''.join(output))
> assert('\'HELLO1\' failed' in ''.join(output))
> diff --git a/test/py/tests/test_efi_secboot/test_signed_intca.py
> b/test/py/tests/test_efi_secboot/test_signed_intca.py
> index d8d599d22f..8d9a5f3e7f 100644
> --- a/test/py/tests/test_efi_secboot/test_signed_intca.py
> +++ b/test/py/tests/test_efi_secboot/test_signed_intca.py
> @@ -40,7 +40,7 @@ class TestEfiSignedImageIntca(object):
>
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO_a host 0:1
> /helloworld.efi.signed_a -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO_a\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -49,7 +49,7 @@ class TestEfiSignedImageIntca(object):
> # Test Case 1b, signed and authenticated by root CA
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 2 HELLO_ab host 0:1
> /helloworld.efi.signed_ab -s ""',
> - 'efidebug boot next 2',
> + 'efidebug boot order 2',
> 'bootefi bootmgr'])
> assert 'Hello, world!' in ''.join(output)
>
> @@ -71,7 +71,7 @@ class TestEfiSignedImageIntca(object):
>
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO_abc host 0:1
> /helloworld.efi.signed_abc -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO_abc\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -81,7 +81,7 @@ class TestEfiSignedImageIntca(object):
> output = u_boot_console.run_command_list([
> 'fatload host 0:1 4000000 db_b.auth',
> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO_abc\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> @@ -91,7 +91,7 @@ class TestEfiSignedImageIntca(object):
> output = u_boot_console.run_command_list([
> 'fatload host 0:1 4000000 db_c.auth',
> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert 'Hello, world!' in ''.join(output)
>
> @@ -117,7 +117,7 @@ class TestEfiSignedImageIntca(object):
>
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO_abc host 0:1
> /helloworld.efi.signed_abc -s ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert 'Hello, world!' in ''.join(output)
> # Or,
> @@ -129,7 +129,7 @@ class TestEfiSignedImageIntca(object):
> output = u_boot_console.run_command_list([
> 'fatload host 0:1 4000000 dbx_c.auth',
> 'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert '\'HELLO_abc\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
> diff --git a/test/py/tests/test_efi_secboot/test_unsigned.py
> b/test/py/tests/test_efi_secboot/test_unsigned.py
> index df63f0df08..7c078f220d 100644
> --- a/test/py/tests/test_efi_secboot/test_unsigned.py
> +++ b/test/py/tests/test_efi_secboot/test_unsigned.py
> @@ -36,11 +36,11 @@ class TestEfiUnsignedImage(object):
>
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi -s
> ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'bootefi bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert 'efi_start_image() returned: 26' in ''.join(output)
> assert 'Hello, world!' not in ''.join(output)
> @@ -65,7 +65,7 @@ class TestEfiUnsignedImage(object):
>
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi -s
> ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'bootefi bootmgr'])
> assert 'Hello, world!' in ''.join(output)
>
> @@ -89,11 +89,11 @@ class TestEfiUnsignedImage(object):
>
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi -s
> ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'bootefi bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert 'efi_start_image() returned: 26' in ''.join(output)
> assert 'Hello, world!' not in ''.join(output)
> @@ -107,11 +107,11 @@ class TestEfiUnsignedImage(object):
>
> output = u_boot_console.run_command_list([
> 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi -s
> ""',
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'bootefi bootmgr'])
> assert '\'HELLO\' failed' in ''.join(output)
> output = u_boot_console.run_command_list([
> - 'efidebug boot next 1',
> + 'efidebug boot order 1',
> 'efidebug test bootmgr'])
> assert 'efi_start_image() returned: 26' in ''.join(output)
> assert 'Hello, world!' not in ''.join(output)
> diff --git a/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py
> b/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py
> index 527a556ed8..3b8118104f 100644
> --- a/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py
> +++ b/test/py/tests/test_fs/test_squashfs/test_sqfs_ls.py
> @@ -118,6 +118,12 @@ def test_sqfs_ls(u_boot_console):
> """
> build_dir = u_boot_console.config.build_dir
>
> + # If the EFI subsystem is enabled, default file(e.g.
> EFI/BOOT/BOOTAA64.EFI)
> + # is scanned when the new disk is detected. This ends up with the
> unexpected
> + # output at the first 'sqfsls' command.
> + # Clear the previous state.
> + u_boot_console.restart_uboot()
> +
> # setup test environment
> check_mksquashfs_version()
> generate_sqfs_src_dir(build_dir)
> --
> 2.34.1
>
>
Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
--
Best regards,
João Marcos Costa
www.linkedin.com/in/jmarcoscosta/
https://github.com/jmarcoscosta
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v7 7/9] Boot var automatic management for removable medias
2023-10-16 7:58 ` João Marcos Costa
@ 2023-10-16 19:02 ` João Marcos Costa
0 siblings, 0 replies; 33+ messages in thread
From: João Marcos Costa @ 2023-10-16 19:02 UTC (permalink / raw)
To: Masahisa Kojima
Cc: u-boot, Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, Raymond Mao, Thomas Petazzoni,
Miquel Raynal
Hello again,
Em seg., 16 de out. de 2023 às 09:58, João Marcos Costa <
jmcosta944@gmail.com> escreveu:
> Hello,
>
> Em seg., 16 de out. de 2023 às 08:47, Masahisa Kojima <
> masahisa.kojima@linaro.org> escreveu:
>
>> From: Raymond Mao <raymond.mao@linaro.org>
>>
>> Changes for complying to EFI spec §3.5.1.1
>> 'Removable Media Boot Behavior'.
>> Boot variables can be automatically generated during a removable
>> media is probed. At the same time, unused boot variables will be
>> detected and removed.
>>
>> Please note that currently the function 'efi_disk_remove' has no
>> ability to distinguish below two scenarios
>> a) Unplugging of a removable media under U-Boot
>> b) U-Boot exiting and booting an OS
>> Thus currently the boot variables management is not added into
>> 'efi_disk_remove' to avoid boot options being added/erased
>> repeatedly under scenario b) during power cycles
>> See TODO comments under function 'efi_disk_remove' for more details
>>
>> The original efi_secboot tests expect that BootOrder EFI variable
>> is not defined. With this commit, the BootOrder EFI variable is
>> automatically added when the disk is detected. The original
>> efi_secboot tests end up with unexpected failure.
>> The efi_secboot tests need to be modified to explicitly set
>> the BootOrder EFI variable.
>>
>> squashfs ls test is also affected by this modification, need to
>> clear the previous state before squashfs ls test starts.
>>
>> Co-developed-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>> Signed-off-by: Raymond Mao <raymond.mao@linaro.org>
>> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> ---
>
> [...]
> Signed-off-by: Joao Marcos Costa <jmcosta944@gmail.com>
>
Sorry for the previous reply containing the wrong tag. Here is my
Reviewed-by tag:
Reviewed-by: Joao Marcos Costa <jmcosta944@gmail.com>
--
Atenciosamente,
João Marcos Costa
www.linkedin.com/in/jmarcoscosta/
https://github.com/jmarcoscosta
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v7 8/9] cmd: efidebug: add uri device path
2023-10-16 6:45 [PATCH v7 0/9] Add EFI HTTP boot support Masahisa Kojima
` (6 preceding siblings ...)
2023-10-16 6:45 ` [PATCH v7 7/9] Boot var automatic management for removable medias Masahisa Kojima
@ 2023-10-16 6:45 ` Masahisa Kojima
2023-10-17 19:27 ` Ilias Apalodimas
2023-10-16 6:45 ` [PATCH v7 9/9] doc: uefi: add HTTP Boot support Masahisa Kojima
8 siblings, 1 reply; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-16 6:45 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, Masahisa Kojima, Joe Hershberger,
Ramon Fried
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 | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 129 insertions(+)
diff --git a/cmd/efidebug.c b/cmd/efidebug.c
index 0be3af3e76..5f392d2ffd 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_EFI_HTTP_BOOT))
+ 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_EFI_HTTP_BOOT))
+ " -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 2087146b37..6ae2237a0a 100644
--- a/net/wget.c
+++ b/net/wget.c
@@ -566,3 +566,74 @@ out:
return ret;
}
#endif
+
+/**
+ * wget_validate_uri() - validate the uri for wget
+ *
+ * @uri: uri string
+ *
+ * This function 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)
+ *
+ * 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.
+ *
+ * TODO: stricter uri conformance check
+ *
+ * Return: true on success, false on failure
+ */
+bool wget_validate_uri(char *uri)
+{
+ char c;
+ bool ret = true;
+ char *str_copy, *s, *authority;
+
+ for (c = 0x1; c < 0x21; c++) {
+ if (strchr(uri, c)) {
+ log_err("invalid character is used\n");
+ return false;
+ }
+ }
+ if (strchr(uri, 0x7f)) {
+ log_err("invalid character is used\n");
+ return false;
+ }
+
+ if (strncmp(uri, "http://", 7)) {
+ log_err("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) {
+ log_err("invalid uri, no file path\n");
+ ret = false;
+ goto out;
+ }
+ s = strchr(authority, '@');
+ if (s) {
+ log_err("user information is not supported\n");
+ ret = false;
+ goto out;
+ }
+ s = strchr(authority, ':');
+ if (s) {
+ log_err("user defined port is not supported\n");
+ ret = false;
+ goto out;
+ }
+
+out:
+ free(str_copy);
+
+ return ret;
+}
--
2.34.1
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v7 8/9] cmd: efidebug: add uri device path
2023-10-16 6:45 ` [PATCH v7 8/9] cmd: efidebug: add uri device path Masahisa Kojima
@ 2023-10-17 19:27 ` Ilias Apalodimas
2023-10-18 8:38 ` Masahisa Kojima
0 siblings, 1 reply; 33+ messages in thread
From: Ilias Apalodimas @ 2023-10-17 19:27 UTC (permalink / raw)
To: Masahisa Kojima
Cc: u-boot, Heinrich Schuchardt, Simon Glass, Takahiro Akashi,
Michal Simek, Joe Hershberger, Ramon Fried
Hi Kojima-san,
[...]
> + 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;
> + }
This needs to be moved right under label = efi_convert_string(argv[2]);
and group all the checks together early. You won't have to allocate and
free the memory as well
> +
> + strcpy(uridp->uri, argv[3]);
> + pos = (char *)uridp + uridp_len;
> + memcpy(pos, &END, sizeof(END));
> + fp_size += uridp_len + sizeof(END);
Assign this earlier and use it on fp_free = efi_alloc(...)
> + file_path = (struct efi_device_path *)uridp;
> + argc -= 3;
[...]
> + *
> + * @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 2087146b37..6ae2237a0a 100644
> --- a/net/wget.c
> +++ b/net/wget.c
> @@ -566,3 +566,74 @@ out:
> return ret;
> }
> #endif
> +
Regards
/Ilias
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v7 8/9] cmd: efidebug: add uri device path
2023-10-17 19:27 ` Ilias Apalodimas
@ 2023-10-18 8:38 ` Masahisa Kojima
0 siblings, 0 replies; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-18 8:38 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: u-boot, Heinrich Schuchardt, Simon Glass, Takahiro Akashi,
Michal Simek, Joe Hershberger, Ramon Fried
Hi Ilias,
On Wed, 18 Oct 2023 at 04:27, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Kojima-san,
>
> [...]
>
> > + 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;
> > + }
>
> This needs to be moved right under label = efi_convert_string(argv[2]);
> and group all the checks together early. You won't have to allocate and
> free the memory as well
OK.
>
> > +
> > + strcpy(uridp->uri, argv[3]);
> > + pos = (char *)uridp + uridp_len;
> > + memcpy(pos, &END, sizeof(END));
> > + fp_size += uridp_len + sizeof(END);
>
> Assign this earlier and use it on fp_free = efi_alloc(...)
OK.
Thanks,
Masahisa Kojima
>
> > + file_path = (struct efi_device_path *)uridp;
> > + argc -= 3;
>
> [...]
>
> > + *
> > + * @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 2087146b37..6ae2237a0a 100644
> > --- a/net/wget.c
> > +++ b/net/wget.c
> > @@ -566,3 +566,74 @@ out:
> > return ret;
> > }
> > #endif
> > +
>
> Regards
> /Ilias
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v7 9/9] doc: uefi: add HTTP Boot support
2023-10-16 6:45 [PATCH v7 0/9] Add EFI HTTP boot support Masahisa Kojima
` (7 preceding siblings ...)
2023-10-16 6:45 ` [PATCH v7 8/9] cmd: efidebug: add uri device path Masahisa Kojima
@ 2023-10-16 6:45 ` Masahisa Kojima
8 siblings, 0 replies; 33+ messages in thread
From: Masahisa Kojima @ 2023-10-16 6:45 UTC (permalink / raw)
To: u-boot
Cc: Heinrich Schuchardt, Ilias Apalodimas, Simon Glass,
Takahiro Akashi, Michal Simek, Masahisa Kojima
This adds the description about HTTP Boot.
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@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 f8510d31d7..d7332c300d 100644
--- a/doc/develop/uefi/uefi.rst
+++ b/doc/develop/uefi/uefi.rst
@@ -625,6 +625,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.
+
+The 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
^ permalink raw reply related [flat|nested] 33+ messages in thread