[v7,4/9] efi_loader: create default file boot option

Message ID	20231016064526.2410856-5-masahisa.kojima@linaro.org
State	New
Headers	show Delivered-To: patch@linaro.org Received-SPF: pass (google.com: domain of u-boot-bounces@lists.denx.de designates 2a01:238:438b:c500:173d:9f52:ddab:ee01 as permitted sender) client-ip=2a01:238:438b:c500:173d:9f52:ddab:ee01; From: Masahisa Kojima <masahisa.kojima@linaro.org> To: u-boot@lists.denx.de Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>, Ilias Apalodimas <ilias.apalodimas@linaro.org>, Simon Glass <sjg@chromium.org>, Takahiro Akashi <takahiro.akashi@linaro.org>, Michal Simek <michal.simek@amd.com>, Masahisa Kojima <masahisa.kojima@linaro.org> Subject: [PATCH v7 4/9] efi_loader: create default file boot option Date: Mon, 16 Oct 2023 15:45:21 +0900 Message-Id: <20231016064526.2410856-5-masahisa.kojima@linaro.org> In-Reply-To: <20231016064526.2410856-1-masahisa.kojima@linaro.org> References: <20231016064526.2410856-1-masahisa.kojima@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: list Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" <u-boot-bounces@lists.denx.de>
Series	Add EFI HTTP boot support \| expand [v7,0/9] Add EFI HTTP boot support [v7,1/9] net: wget: prevent overwriting reserved memory [v7,2/9] net: wget: add wget with dns utility function [v7,3/9] blk: blkmap: add ramdisk creation utility function [v7,4/9] efi_loader: create default file boot option [v7,5/9] efi_loader: support boot from URI device path [v7,6/9] efi_loader: add CDROM short-form device path [v7,7/9] Boot var automatic management for removable medias [v7,8/9] cmd: efidebug: add uri device path [v7,9/9] doc: uefi: add HTTP Boot support

Masahisa Kojima Oct. 16, 2023, 6:45 a.m. UTC

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

Heinrich Schuchardt Oct. 16, 2023, 7:06 a.m. UTC | #1

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

Ilias Apalodimas Oct. 16, 2023, 12:31 p.m. UTC | #2

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

Heinrich Schuchardt Oct. 16, 2023, 12:46 p.m. UTC | #3

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

Ilias Apalodimas Oct. 16, 2023, 12:57 p.m. UTC | #4

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

Masahisa Kojima Oct. 16, 2023, 1 p.m. UTC | #5

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

Heinrich Schuchardt Oct. 16, 2023, 2:47 p.m. UTC | #6

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

Masahisa Kojima Oct. 17, 2023, 5:32 a.m. UTC | #7

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

Ilias Apalodimas Oct. 17, 2023, 7:46 p.m. UTC | #8

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

Heinrich Schuchardt Oct. 17, 2023, 8:29 p.m. UTC | #9

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

Masahisa Kojima Oct. 18, 2023, 9:07 a.m. UTC | #10

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

Ilias Apalodimas Oct. 18, 2023, 9:12 a.m. UTC | #11

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

Heinrich Schuchardt Oct. 18, 2023, 9:30 a.m. UTC | #12

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

Masahisa Kojima Oct. 19, 2023, 6:45 a.m. UTC | #13

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

Ilias Apalodimas Oct. 19, 2023, 7:22 a.m. UTC | #14

[...]

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

Masahisa Kojima Oct. 19, 2023, 8:24 a.m. UTC | #15

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

[v7,4/9] efi_loader: create default file boot option

Commit Message

Comments

Patch