Hi Heinrich, On Mon, 16 Oct 2023 at 23:52, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > On 16.10.23 15:00, Masahisa Kojima wrote: > > On Mon, 16 Oct 2023 at 21:46, Heinrich Schuchardt <xypron.g...@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.g...@gmx.de> > >>> wrote: > >>>> > >>>> > >>>> > >>>> Am 16. Oktober 2023 08:45:21 MESZ schrieb Masahisa Kojima > >>>> <masahisa.koj...@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.koj...@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; > >>>>> > >> >
