* [RFC 0/3] efi_loader: bootmgr itself should support removable media @ 2021-11-09 1:32 AKASHI Takahiro 2021-11-09 1:32 ` [RFC 1/3] efi_loader: export efi_locate_device_handle() AKASHI Takahiro ` (2 more replies) 0 siblings, 3 replies; 13+ messages in thread From: AKASHI Takahiro @ 2021-11-09 1:32 UTC (permalink / raw) To: xypron.glpk, agraf; +Cc: sjg, ilias.apalodimas, u-boot, AKASHI Takahiro # This task is a bit motivated by Simon's discussion about bootflow[1]. Booting UEFI system from removable media is currently supported by distro_bootcmd script. But the behavior should be best implemented in UEFI Boot Manager. (Historically, the boot manager was added later than the support for removable media boot. So it is a matter of integration.) Here, "removable media" support means: 3.5.1.1 Removable Media Boot Behavior To generate a file name when none is present in the FilePath, the firmware must append a default file name in the form \EFI\BOOT\BOOT{machine type short-name}.EFI where machine type short-name defines a PE32+ image format architecture. With this patch set applied, the boot media order can also be defined by "BooOrder" variable, which the UEFI specification expects. You can find an example command usage in patch#3. Please note that this ehnancement improves the compliance to the specification and that it won't hurt any backward compatibility, if you don't want to use this feature, as long as a removable-related script in distro_bootcmd remains unchanged. The restriction (or drawback?) here is that we need to run all the "scan" commands before invoking the boot manager. [1] https://lists.denx.de/pipermail/u-boot/2021-August/458384.html RFC (Nov 09, 2021) * initial proposal AKASHI Takahiro (3): efi_loader: export efi_locate_device_handle() efi_loader: bootmgr: add booting from removable media cmd: efidebug: handle booting from removable media cmd/efidebug.c | 46 +++++++++++++++++++++---- include/efi_loader.h | 4 +++ lib/efi_loader/efi_bootmgr.c | 65 ++++++++++++++++++++++++++++++++++- lib/efi_loader/efi_boottime.c | 7 ++-- 4 files changed, 111 insertions(+), 11 deletions(-) -- 2.33.0 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC 1/3] efi_loader: export efi_locate_device_handle() 2021-11-09 1:32 [RFC 0/3] efi_loader: bootmgr itself should support removable media AKASHI Takahiro @ 2021-11-09 1:32 ` AKASHI Takahiro 2021-11-09 1:32 ` [RFC 2/3] efi_loader: bootmgr: add booting from removable media AKASHI Takahiro 2021-11-09 1:32 ` [RFC 3/3] cmd: efidebug: handle " AKASHI Takahiro 2 siblings, 0 replies; 13+ messages in thread From: AKASHI Takahiro @ 2021-11-09 1:32 UTC (permalink / raw) To: xypron.glpk, agraf; +Cc: sjg, ilias.apalodimas, u-boot, AKASHI Takahiro This function will be used in the next commit where some behavior of EFI boot manager will be expanded. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/efi_loader.h | 4 ++++ lib/efi_loader/efi_boottime.c | 7 +++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/include/efi_loader.h b/include/efi_loader.h index d52e399841ba..65236fa64616 100644 --- a/include/efi_loader.h +++ b/include/efi_loader.h @@ -587,6 +587,10 @@ efi_status_t efi_create_handle(efi_handle_t *handle); void efi_delete_handle(efi_handle_t obj); /* Call this to validate a handle and find the EFI object for it */ struct efi_object *efi_search_obj(const efi_handle_t handle); +/* Locate device_path handle */ +efi_status_t EFIAPI efi_locate_device_path(const efi_guid_t *protocol, + struct efi_device_path **device_path, + efi_handle_t *device); /* Load image */ efi_status_t EFIAPI efi_load_image(bool boot_policy, efi_handle_t parent_image, diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c index 1823990d9bd5..d5b764e1a1e1 100644 --- a/lib/efi_loader/efi_boottime.c +++ b/lib/efi_loader/efi_boottime.c @@ -1796,10 +1796,9 @@ failure: * * Return: status code */ -static efi_status_t EFIAPI efi_locate_device_path( - const efi_guid_t *protocol, - struct efi_device_path **device_path, - efi_handle_t *device) +efi_status_t EFIAPI efi_locate_device_path(const efi_guid_t *protocol, + struct efi_device_path **device_path, + efi_handle_t *device) { struct efi_device_path *dp; size_t i; -- 2.33.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [RFC 2/3] efi_loader: bootmgr: add booting from removable media 2021-11-09 1:32 [RFC 0/3] efi_loader: bootmgr itself should support removable media AKASHI Takahiro 2021-11-09 1:32 ` [RFC 1/3] efi_loader: export efi_locate_device_handle() AKASHI Takahiro @ 2021-11-09 1:32 ` AKASHI Takahiro 2021-11-09 9:14 ` Heinrich Schuchardt 2021-11-09 13:53 ` Mark Kettenis 2021-11-09 1:32 ` [RFC 3/3] cmd: efidebug: handle " AKASHI Takahiro 2 siblings, 2 replies; 13+ messages in thread From: AKASHI Takahiro @ 2021-11-09 1:32 UTC (permalink / raw) To: xypron.glpk, agraf; +Cc: sjg, ilias.apalodimas, u-boot, AKASHI Takahiro Under the current implementation, booting from removal media using a architecture-specific default image name, say BOOTAA64.EFI, is supported only in distro_bootcmd script. See the commit 74522c898b35 ("efi_loader: Add distro boot script for removable media"). This is, however, half-baked implementation because 1) UEFI specification requires this feature to be implemented as part of Boot Manager's responsibility: 3 - Boot Manager 3.5.1 Boot via the Simple File Protocol When booting via the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, the FilePath will start with a device path that points to the device that implements the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL or the EFI_BLOCK_IO_PROTOCOL. The next part of the FilePath may point to the file name, including subdirectories, which contain the bootable image. If the file name is a null device path, the file name must be generated from the rules defined below. ... 3.5.1.1 Removable Media Boot Behavior To generate a file name when none is present in the FilePath, the firmware must append a default file name in the form \EFI\BOOT\BOOT{machine type short-name}.EFI ... 2) So (1) entails the hehavior that the user's preference of boot media order should be determined by Boot#### and BootOrder variables. With this patch, the semantics mentioned above is fully implemented. For example, if you want to boot the system from USB and SCSI in this order, * define Boot0001 which contains only a device path to the USB device (without any file path/name) * define Boot0002 which contains only a device path to the SCSI device, and * set BootOrder to Boot0001:Boot0002 Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- lib/efi_loader/efi_bootmgr.c | 65 +++++++++++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 1fe19237f9a6..1d9d5858561f 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -30,6 +30,66 @@ static const struct efi_runtime_services *rs; * should do normal or recovery boot. */ +#if defined(CONFIG_ARM64) +#define BOOTEFI_NAME "bootaa64.efi" +#elif defined(CONFIG_ARM) +#define BOOTEFI_NAME "bootarm.efi" +#elif defined(CONFIG_X86_RUN_32BIT) +#define BOOTEFI_NAME "bootia32.efi" +#elif defined(CONFIG_X86_RUN_64BIT) +#define BOOTEFI_NAME "bootx64.efi" +#elif defined(CONFIG_ARCH_RV32I) +#define BOOTEFI_NAME "bootriscv32.efi" +#elif defined(CONFIG_ARCH_RV64I) +#define BOOTEFI_NAME "bootriscv64.efi" +#else +#define BOOTEFI_NAME "dummy.efi" +#endif + +/** + * expand_media_path() - expand a device path for default file name + * @device_path: device path to check against + * + * If @device_path is a media or disk partition which houses a file + * system, this function returns a full device path which contains + * an architecture-specific default file name for removable media. + * + * Return: a newly allocated device path + */ +static +struct efi_device_path *expand_media_path(struct efi_device_path *device_path) +{ + struct efi_device_path *dp, *full_path; + efi_handle_t handle; + efi_status_t ret; + + if (!device_path) + return NULL; + + /* + * If device_path is a (removable) media or partition which provides + * simple file system protocol, append a default file name to support + * booting from removable media. + */ + dp = device_path; + ret = efi_locate_device_path(&efi_simple_file_system_protocol_guid, + &dp, &handle); + if (ret == EFI_SUCCESS) { + if (dp->type == DEVICE_PATH_TYPE_END) { + dp = efi_dp_from_file(NULL, 0, + "/efi/boot/" BOOTEFI_NAME); + full_path = efi_dp_append(device_path, dp); + } else { + full_path = efi_dp_dup(device_path); + } + efi_free_pool(dp); + } else { + full_path = efi_dp_dup(device_path); + } + + return full_path; +} + /** * try_load_entry() - try to load image for boot option * @@ -68,13 +128,16 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, } if (lo.attributes & LOAD_OPTION_ACTIVE) { + struct efi_device_path *file_path; u32 attributes; log_debug("%s: trying to load \"%ls\" from %pD\n", __func__, lo.label, lo.file_path); - ret = EFI_CALL(efi_load_image(true, efi_root, lo.file_path, + file_path = expand_media_path(lo.file_path); + ret = EFI_CALL(efi_load_image(true, efi_root, file_path, NULL, 0, handle)); + efi_free_pool(file_path); if (ret != EFI_SUCCESS) { log_warning("Loading %ls '%ls' failed\n", varname, lo.label); -- 2.33.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC 2/3] efi_loader: bootmgr: add booting from removable media 2021-11-09 1:32 ` [RFC 2/3] efi_loader: bootmgr: add booting from removable media AKASHI Takahiro @ 2021-11-09 9:14 ` Heinrich Schuchardt 2021-11-10 5:38 ` AKASHI Takahiro 2021-11-09 13:53 ` Mark Kettenis 1 sibling, 1 reply; 13+ messages in thread From: Heinrich Schuchardt @ 2021-11-09 9:14 UTC (permalink / raw) To: AKASHI Takahiro, agraf; +Cc: sjg, ilias.apalodimas, u-boot On 11/9/21 02:32, AKASHI Takahiro wrote: > Under the current implementation, booting from removal media using > a architecture-specific default image name, say BOOTAA64.EFI, is > supported only in distro_bootcmd script. See the commit 74522c898b35 > ("efi_loader: Add distro boot script for removable media"). > > This is, however, half-baked implementation because > 1) UEFI specification requires this feature to be implemented as part > of Boot Manager's responsibility: > > 3 - Boot Manager > 3.5.1 Boot via the Simple File Protocol > When booting via the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, the FilePath will > start with a device path that points to the device that implements the > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL or the EFI_BLOCK_IO_PROTOCOL. The next > part of the FilePath may point to the file name, including > subdirectories, which contain the bootable image. If the file name is > a null device path, the file name must be generated from the rules > defined below. > ... > 3.5.1.1 Removable Media Boot Behavior > To generate a file name when none is present in the FilePath, the > firmware must append a default file name in the form > \EFI\BOOT\BOOT{machine type short-name}.EFI ... > > 2) So (1) entails the hehavior that the user's preference of boot media > order should be determined by Boot#### and BootOrder variables. > > With this patch, the semantics mentioned above is fully implemented. > For example, if you want to boot the system from USB and SCSI in this > order, > * define Boot0001 which contains only a device path to the USB device > (without any file path/name) > * define Boot0002 which contains only a device path to the SCSI device, > and > * set BootOrder to Boot0001:Boot0002 > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_bootmgr.c | 65 +++++++++++++++++++++++++++++++++++- > 1 file changed, 64 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > index 1fe19237f9a6..1d9d5858561f 100644 > --- a/lib/efi_loader/efi_bootmgr.c > +++ b/lib/efi_loader/efi_bootmgr.c > @@ -30,6 +30,66 @@ static const struct efi_runtime_services *rs; > * should do normal or recovery boot. > */ > > +#if defined(CONFIG_ARM64) > +#define BOOTEFI_NAME "bootaa64.efi" > +#elif defined(CONFIG_ARM) > +#define BOOTEFI_NAME "bootarm.efi" > +#elif defined(CONFIG_X86_RUN_32BIT) CONFIG_X86. Check it after CONFIG_X86_64. > +#define BOOTEFI_NAME "bootia32.efi" > +#elif defined(CONFIG_X86_RUN_64BIT) %s/X86_RUN_64BIT/X86_64/ Check this before CONFIG_X86 > +#define BOOTEFI_NAME "bootx64.efi" > +#elif defined(CONFIG_ARCH_RV32I) > +#define BOOTEFI_NAME "bootriscv32.efi" > +#elif defined(CONFIG_ARCH_RV64I) > +#define BOOTEFI_NAME "bootriscv64.efi" > +#else > +#define BOOTEFI_NAME "dummy.efi" This should result in a build error. #error Unsupported UEFI architecture > +#endif I think this belongs into an include file. According to the UEFI specification the file names are capitalized. > + > +/** > + * expand_media_path() - expand a device path for default file name > + * @device_path: device path to check against > + * > + * If @device_path is a media or disk partition which houses a file > + * system, this function returns a full device path which contains > + * an architecture-specific default file name for removable media. > + * > + * Return: a newly allocated device path > + */ > +static > +struct efi_device_path *expand_media_path(struct efi_device_path *device_path) > +{ > + struct efi_device_path *dp, *full_path; > + efi_handle_t handle; > + efi_status_t ret; > + > + if (!device_path) > + return NULL; > + > + /* > + * If device_path is a (removable) media or partition which provides > + * simple file system protocol, append a default file name to support > + * booting from removable media. > + */ > + dp = device_path; > + ret = efi_locate_device_path(&efi_simple_file_system_protocol_guid, > + &dp, &handle); > + if (ret == EFI_SUCCESS) { > + if (dp->type == DEVICE_PATH_TYPE_END) { > + dp = efi_dp_from_file(NULL, 0, > + "/efi/boot/" BOOTEFI_NAME); capitalized: /EFI/BOOT/ Best regards Heinrich > + full_path = efi_dp_append(device_path, dp); > + } else { > + full_path = efi_dp_dup(device_path); > + } > + efi_free_pool(dp); > + } else { > + full_path = efi_dp_dup(device_path); > + } > + > + return full_path; > +} > + > /** > * try_load_entry() - try to load image for boot option > * > @@ -68,13 +128,16 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, > } > > if (lo.attributes & LOAD_OPTION_ACTIVE) { > + struct efi_device_path *file_path; > u32 attributes; > > log_debug("%s: trying to load \"%ls\" from %pD\n", > __func__, lo.label, lo.file_path); > > - ret = EFI_CALL(efi_load_image(true, efi_root, lo.file_path, > + file_path = expand_media_path(lo.file_path); > + ret = EFI_CALL(efi_load_image(true, efi_root, file_path, > NULL, 0, handle)); > + efi_free_pool(file_path); > if (ret != EFI_SUCCESS) { > log_warning("Loading %ls '%ls' failed\n", > varname, lo.label); > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC 2/3] efi_loader: bootmgr: add booting from removable media 2021-11-09 9:14 ` Heinrich Schuchardt @ 2021-11-10 5:38 ` AKASHI Takahiro 0 siblings, 0 replies; 13+ messages in thread From: AKASHI Takahiro @ 2021-11-10 5:38 UTC (permalink / raw) To: Heinrich Schuchardt; +Cc: agraf, sjg, ilias.apalodimas, u-boot Hi Heinrich, Overall, do you agree to add this feature? On Tue, Nov 09, 2021 at 10:14:47AM +0100, Heinrich Schuchardt wrote: > > > On 11/9/21 02:32, AKASHI Takahiro wrote: > > Under the current implementation, booting from removal media using > > a architecture-specific default image name, say BOOTAA64.EFI, is > > supported only in distro_bootcmd script. See the commit 74522c898b35 > > ("efi_loader: Add distro boot script for removable media"). > > > > This is, however, half-baked implementation because > > 1) UEFI specification requires this feature to be implemented as part > > of Boot Manager's responsibility: > > > > 3 - Boot Manager > > 3.5.1 Boot via the Simple File Protocol > > When booting via the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, the FilePath will > > start with a device path that points to the device that implements the > > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL or the EFI_BLOCK_IO_PROTOCOL. The next > > part of the FilePath may point to the file name, including > > subdirectories, which contain the bootable image. If the file name is > > a null device path, the file name must be generated from the rules > > defined below. > > ... > > 3.5.1.1 Removable Media Boot Behavior > > To generate a file name when none is present in the FilePath, the > > firmware must append a default file name in the form > > \EFI\BOOT\BOOT{machine type short-name}.EFI ... > > > > 2) So (1) entails the hehavior that the user's preference of boot media > > order should be determined by Boot#### and BootOrder variables. > > > > With this patch, the semantics mentioned above is fully implemented. > > For example, if you want to boot the system from USB and SCSI in this > > order, > > * define Boot0001 which contains only a device path to the USB device > > (without any file path/name) > > * define Boot0002 which contains only a device path to the SCSI device, > > and > > * set BootOrder to Boot0001:Boot0002 > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > lib/efi_loader/efi_bootmgr.c | 65 +++++++++++++++++++++++++++++++++++- > > 1 file changed, 64 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > > index 1fe19237f9a6..1d9d5858561f 100644 > > --- a/lib/efi_loader/efi_bootmgr.c > > +++ b/lib/efi_loader/efi_bootmgr.c > > @@ -30,6 +30,66 @@ static const struct efi_runtime_services *rs; > > * should do normal or recovery boot. > > */ > > +#if defined(CONFIG_ARM64) > > +#define BOOTEFI_NAME "bootaa64.efi" > > +#elif defined(CONFIG_ARM) > > +#define BOOTEFI_NAME "bootarm.efi" > > +#elif defined(CONFIG_X86_RUN_32BIT) > > CONFIG_X86. Check it after CONFIG_X86_64. > > > +#define BOOTEFI_NAME "bootia32.efi" > > +#elif defined(CONFIG_X86_RUN_64BIT) > > %s/X86_RUN_64BIT/X86_64/ > > Check this before CONFIG_X86 OK, so #elif defined(CONFIG_X86_64) ... #elif defined(CONFIG_X86) ... > > +#define BOOTEFI_NAME "bootx64.efi" > > +#elif defined(CONFIG_ARCH_RV32I) > > +#define BOOTEFI_NAME "bootriscv32.efi" > > +#elif defined(CONFIG_ARCH_RV64I) > > +#define BOOTEFI_NAME "bootriscv64.efi" > > +#else > > +#define BOOTEFI_NAME "dummy.efi" > > This should result in a build error. > > #error Unsupported UEFI architecture OK. > > +#endif > > I think this belongs into an include file. OK, but the only drawback is that this will make efi_api.h, which I would like to move those definitions to, target-dependent. As we have discussed somewhere else[1], if we want to use those include files, we should remove sich dependencies. Can you also make your comment on this thread[1], please? [1] https://lists.denx.de/pipermail/u-boot/2021-November/466465.html > According to the UEFI specification the file names are capitalized. OK. > > + > > +/** > > + * expand_media_path() - expand a device path for default file name > > + * @device_path: device path to check against > > + * > > + * If @device_path is a media or disk partition which houses a file > > + * system, this function returns a full device path which contains > > + * an architecture-specific default file name for removable media. > > + * > > + * Return: a newly allocated device path > > + */ > > +static > > +struct efi_device_path *expand_media_path(struct efi_device_path *device_path) > > +{ > > + struct efi_device_path *dp, *full_path; > > + efi_handle_t handle; > > + efi_status_t ret; > > + > > + if (!device_path) > > + return NULL; > > + > > + /* > > + * If device_path is a (removable) media or partition which provides > > + * simple file system protocol, append a default file name to support > > + * booting from removable media. > > + */ > > + dp = device_path; > > + ret = efi_locate_device_path(&efi_simple_file_system_protocol_guid, > > + &dp, &handle); > > + if (ret == EFI_SUCCESS) { > > + if (dp->type == DEVICE_PATH_TYPE_END) { > > + dp = efi_dp_from_file(NULL, 0, > > + "/efi/boot/" BOOTEFI_NAME); > > capitalized: > /EFI/BOOT/ OK -Takahiro Akashi > Best regards > > Heinrich > > > + full_path = efi_dp_append(device_path, dp); > > + } else { > > + full_path = efi_dp_dup(device_path); > > + } > > + efi_free_pool(dp); > > + } else { > > + full_path = efi_dp_dup(device_path); > > + } > > + > > + return full_path; > > +} > > + > > /** > > * try_load_entry() - try to load image for boot option > > * > > @@ -68,13 +128,16 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, > > } > > if (lo.attributes & LOAD_OPTION_ACTIVE) { > > + struct efi_device_path *file_path; > > u32 attributes; > > log_debug("%s: trying to load \"%ls\" from %pD\n", > > __func__, lo.label, lo.file_path); > > - ret = EFI_CALL(efi_load_image(true, efi_root, lo.file_path, > > + file_path = expand_media_path(lo.file_path); > > + ret = EFI_CALL(efi_load_image(true, efi_root, file_path, > > NULL, 0, handle)); > > + efi_free_pool(file_path); > > if (ret != EFI_SUCCESS) { > > log_warning("Loading %ls '%ls' failed\n", > > varname, lo.label); > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC 2/3] efi_loader: bootmgr: add booting from removable media 2021-11-09 1:32 ` [RFC 2/3] efi_loader: bootmgr: add booting from removable media AKASHI Takahiro 2021-11-09 9:14 ` Heinrich Schuchardt @ 2021-11-09 13:53 ` Mark Kettenis 2021-11-10 5:52 ` AKASHI Takahiro 1 sibling, 1 reply; 13+ messages in thread From: Mark Kettenis @ 2021-11-09 13:53 UTC (permalink / raw) To: AKASHI Takahiro Cc: xypron.glpk, agraf, sjg, ilias.apalodimas, u-boot, takahiro.akashi > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > Date: Tue, 9 Nov 2021 10:32:32 +0900 Hi Takahiro, > Under the current implementation, booting from removal media using > a architecture-specific default image name, say BOOTAA64.EFI, is > supported only in distro_bootcmd script. See the commit 74522c898b35 > ("efi_loader: Add distro boot script for removable media"). > > This is, however, half-baked implementation because > 1) UEFI specification requires this feature to be implemented as part > of Boot Manager's responsibility: > > 3 - Boot Manager > 3.5.1 Boot via the Simple File Protocol > When booting via the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, the FilePath will > start with a device path that points to the device that implements the > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL or the EFI_BLOCK_IO_PROTOCOL. The next > part of the FilePath may point to the file name, including > subdirectories, which contain the bootable image. If the file name is > a null device path, the file name must be generated from the rules > defined below. > ... > 3.5.1.1 Removable Media Boot Behavior > To generate a file name when none is present in the FilePath, the > firmware must append a default file name in the form > \EFI\BOOT\BOOT{machine type short-name}.EFI ... > > 2) So (1) entails the hehavior that the user's preference of boot media > order should be determined by Boot#### and BootOrder variables. > > With this patch, the semantics mentioned above is fully implemented. > For example, if you want to boot the system from USB and SCSI in this > order, > * define Boot0001 which contains only a device path to the USB device > (without any file path/name) > * define Boot0002 which contains only a device path to the SCSI device, > and > * set BootOrder to Boot0001:Boot0002 The problem with this approach is that EFI device paths are complicated things and I don't think we can expect users to do what you describe above all by hand. All x86 machines I've seen have a more user-friendly way to configure the desired boot order, and also a way to specify which device to boot from after automatic boot has been interrupted. With the current distroboot scripts one can simply do: setenv boot_targets usb0 mmc0 pxe dhcp and I think we need to have something simple like that. Maybe the code can automatically create Boot#### and BootOrder variables based on the boot_targets environment variables if they're not defined? Another idea is to accept U-Boot device names (e.g. usb0, mmc0) as aliases for EFI device paths and automatically expand them? > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > lib/efi_loader/efi_bootmgr.c | 65 +++++++++++++++++++++++++++++++++++- > 1 file changed, 64 insertions(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > index 1fe19237f9a6..1d9d5858561f 100644 > --- a/lib/efi_loader/efi_bootmgr.c > +++ b/lib/efi_loader/efi_bootmgr.c > @@ -30,6 +30,66 @@ static const struct efi_runtime_services *rs; > * should do normal or recovery boot. > */ > > +#if defined(CONFIG_ARM64) > +#define BOOTEFI_NAME "bootaa64.efi" > +#elif defined(CONFIG_ARM) > +#define BOOTEFI_NAME "bootarm.efi" > +#elif defined(CONFIG_X86_RUN_32BIT) > +#define BOOTEFI_NAME "bootia32.efi" > +#elif defined(CONFIG_X86_RUN_64BIT) > +#define BOOTEFI_NAME "bootx64.efi" > +#elif defined(CONFIG_ARCH_RV32I) > +#define BOOTEFI_NAME "bootriscv32.efi" > +#elif defined(CONFIG_ARCH_RV64I) > +#define BOOTEFI_NAME "bootriscv64.efi" > +#else > +#define BOOTEFI_NAME "dummy.efi" > +#endif > + > +/** > + * expand_media_path() - expand a device path for default file name > + * @device_path: device path to check against > + * > + * If @device_path is a media or disk partition which houses a file > + * system, this function returns a full device path which contains > + * an architecture-specific default file name for removable media. > + * > + * Return: a newly allocated device path > + */ > +static > +struct efi_device_path *expand_media_path(struct efi_device_path *device_path) > +{ > + struct efi_device_path *dp, *full_path; > + efi_handle_t handle; > + efi_status_t ret; > + > + if (!device_path) > + return NULL; > + > + /* > + * If device_path is a (removable) media or partition which provides > + * simple file system protocol, append a default file name to support > + * booting from removable media. > + */ > + dp = device_path; > + ret = efi_locate_device_path(&efi_simple_file_system_protocol_guid, > + &dp, &handle); > + if (ret == EFI_SUCCESS) { > + if (dp->type == DEVICE_PATH_TYPE_END) { > + dp = efi_dp_from_file(NULL, 0, > + "/efi/boot/" BOOTEFI_NAME); > + full_path = efi_dp_append(device_path, dp); > + } else { > + full_path = efi_dp_dup(device_path); > + } > + efi_free_pool(dp); > + } else { > + full_path = efi_dp_dup(device_path); > + } > + > + return full_path; > +} > + > /** > * try_load_entry() - try to load image for boot option > * > @@ -68,13 +128,16 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, > } > > if (lo.attributes & LOAD_OPTION_ACTIVE) { > + struct efi_device_path *file_path; > u32 attributes; > > log_debug("%s: trying to load \"%ls\" from %pD\n", > __func__, lo.label, lo.file_path); > > - ret = EFI_CALL(efi_load_image(true, efi_root, lo.file_path, > + file_path = expand_media_path(lo.file_path); > + ret = EFI_CALL(efi_load_image(true, efi_root, file_path, > NULL, 0, handle)); > + efi_free_pool(file_path); > if (ret != EFI_SUCCESS) { > log_warning("Loading %ls '%ls' failed\n", > varname, lo.label); > -- > 2.33.0 > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC 2/3] efi_loader: bootmgr: add booting from removable media 2021-11-09 13:53 ` Mark Kettenis @ 2021-11-10 5:52 ` AKASHI Takahiro 0 siblings, 0 replies; 13+ messages in thread From: AKASHI Takahiro @ 2021-11-10 5:52 UTC (permalink / raw) To: Mark Kettenis; +Cc: xypron.glpk, agraf, sjg, ilias.apalodimas, u-boot Hi Mark, Thank you for your comments. On Tue, Nov 09, 2021 at 02:53:14PM +0100, Mark Kettenis wrote: > > From: AKASHI Takahiro <takahiro.akashi@linaro.org> > > Date: Tue, 9 Nov 2021 10:32:32 +0900 > > Hi Takahiro, > > > Under the current implementation, booting from removal media using > > a architecture-specific default image name, say BOOTAA64.EFI, is > > supported only in distro_bootcmd script. See the commit 74522c898b35 > > ("efi_loader: Add distro boot script for removable media"). > > > > This is, however, half-baked implementation because > > 1) UEFI specification requires this feature to be implemented as part > > of Boot Manager's responsibility: > > > > 3 - Boot Manager > > 3.5.1 Boot via the Simple File Protocol > > When booting via the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, the FilePath will > > start with a device path that points to the device that implements the > > EFI_SIMPLE_FILE_SYSTEM_PROTOCOL or the EFI_BLOCK_IO_PROTOCOL. The next > > part of the FilePath may point to the file name, including > > subdirectories, which contain the bootable image. If the file name is > > a null device path, the file name must be generated from the rules > > defined below. > > ... > > 3.5.1.1 Removable Media Boot Behavior > > To generate a file name when none is present in the FilePath, the > > firmware must append a default file name in the form > > \EFI\BOOT\BOOT{machine type short-name}.EFI ... > > > > 2) So (1) entails the hehavior that the user's preference of boot media > > order should be determined by Boot#### and BootOrder variables. > > > > With this patch, the semantics mentioned above is fully implemented. > > For example, if you want to boot the system from USB and SCSI in this > > order, > > * define Boot0001 which contains only a device path to the USB device > > (without any file path/name) > > * define Boot0002 which contains only a device path to the SCSI device, > > and > > * set BootOrder to Boot0001:Boot0002 > > The problem with this approach is that EFI device paths are > complicated things and I don't think we can expect users to do what > you describe above all by hand. All x86 machines I've seen have a > more user-friendly way to configure the desired boot order, and also a > way to specify which device to boot from after automatic boot has been > interrupted. I know most x86 prodcuction systems have GUI's, but such interfaces can be implemented on top of the current UEFI implementation. One of candidates, if not the best, is EDK-II's UiApp which is expected to provide more friendly CUI. We'd better move this way. (I mean that we should implement UI applications rather than expand U-Boot itself.) UiApp, however, doesn't work for now on U-Boot due to some issue. > With the current distroboot scripts one can simply do: > > setenv boot_targets usb0 mmc0 pxe dhcp > > and I think we need to have something simple like that. Maybe the > code can automatically create Boot#### and BootOrder variables based > on the boot_targets environment variables if they're not defined? > > Another idea is to accept U-Boot device names (e.g. usb0, mmc0) as > aliases for EFI device paths and automatically expand them? Yeah, those ideas are worth considering, but there are some problems in generating internal device paths for them. Let's continue to discuss this topic in patch#3 thread. Thanks, -Takahiro Akashi > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > lib/efi_loader/efi_bootmgr.c | 65 +++++++++++++++++++++++++++++++++++- > > 1 file changed, 64 insertions(+), 1 deletion(-) > > > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > > index 1fe19237f9a6..1d9d5858561f 100644 > > --- a/lib/efi_loader/efi_bootmgr.c > > +++ b/lib/efi_loader/efi_bootmgr.c > > @@ -30,6 +30,66 @@ static const struct efi_runtime_services *rs; > > * should do normal or recovery boot. > > */ > > > > +#if defined(CONFIG_ARM64) > > +#define BOOTEFI_NAME "bootaa64.efi" > > +#elif defined(CONFIG_ARM) > > +#define BOOTEFI_NAME "bootarm.efi" > > +#elif defined(CONFIG_X86_RUN_32BIT) > > +#define BOOTEFI_NAME "bootia32.efi" > > +#elif defined(CONFIG_X86_RUN_64BIT) > > +#define BOOTEFI_NAME "bootx64.efi" > > +#elif defined(CONFIG_ARCH_RV32I) > > +#define BOOTEFI_NAME "bootriscv32.efi" > > +#elif defined(CONFIG_ARCH_RV64I) > > +#define BOOTEFI_NAME "bootriscv64.efi" > > +#else > > +#define BOOTEFI_NAME "dummy.efi" > > +#endif > > + > > +/** > > + * expand_media_path() - expand a device path for default file name > > + * @device_path: device path to check against > > + * > > + * If @device_path is a media or disk partition which houses a file > > + * system, this function returns a full device path which contains > > + * an architecture-specific default file name for removable media. > > + * > > + * Return: a newly allocated device path > > + */ > > +static > > +struct efi_device_path *expand_media_path(struct efi_device_path *device_path) > > +{ > > + struct efi_device_path *dp, *full_path; > > + efi_handle_t handle; > > + efi_status_t ret; > > + > > + if (!device_path) > > + return NULL; > > + > > + /* > > + * If device_path is a (removable) media or partition which provides > > + * simple file system protocol, append a default file name to support > > + * booting from removable media. > > + */ > > + dp = device_path; > > + ret = efi_locate_device_path(&efi_simple_file_system_protocol_guid, > > + &dp, &handle); > > + if (ret == EFI_SUCCESS) { > > + if (dp->type == DEVICE_PATH_TYPE_END) { > > + dp = efi_dp_from_file(NULL, 0, > > + "/efi/boot/" BOOTEFI_NAME); > > + full_path = efi_dp_append(device_path, dp); > > + } else { > > + full_path = efi_dp_dup(device_path); > > + } > > + efi_free_pool(dp); > > + } else { > > + full_path = efi_dp_dup(device_path); > > + } > > + > > + return full_path; > > +} > > + > > /** > > * try_load_entry() - try to load image for boot option > > * > > @@ -68,13 +128,16 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle, > > } > > > > if (lo.attributes & LOAD_OPTION_ACTIVE) { > > + struct efi_device_path *file_path; > > u32 attributes; > > > > log_debug("%s: trying to load \"%ls\" from %pD\n", > > __func__, lo.label, lo.file_path); > > > > - ret = EFI_CALL(efi_load_image(true, efi_root, lo.file_path, > > + file_path = expand_media_path(lo.file_path); > > + ret = EFI_CALL(efi_load_image(true, efi_root, file_path, > > NULL, 0, handle)); > > + efi_free_pool(file_path); > > if (ret != EFI_SUCCESS) { > > log_warning("Loading %ls '%ls' failed\n", > > varname, lo.label); > > -- > > 2.33.0 > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [RFC 3/3] cmd: efidebug: handle booting from removable media 2021-11-09 1:32 [RFC 0/3] efi_loader: bootmgr itself should support removable media AKASHI Takahiro 2021-11-09 1:32 ` [RFC 1/3] efi_loader: export efi_locate_device_handle() AKASHI Takahiro 2021-11-09 1:32 ` [RFC 2/3] efi_loader: bootmgr: add booting from removable media AKASHI Takahiro @ 2021-11-09 1:32 ` AKASHI Takahiro 2021-11-09 9:50 ` Heinrich Schuchardt 2 siblings, 1 reply; 13+ messages in thread From: AKASHI Takahiro @ 2021-11-09 1:32 UTC (permalink / raw) To: xypron.glpk, agraf; +Cc: sjg, ilias.apalodimas, u-boot, AKASHI Takahiro Support for booting from removable media is now added to UEFI boot manager. Here we should modify efidebug command in order to define a proper "BootXXXX" variable. With this patch applied, you will be able to specify the boot order, usb and scsi: => efidebug -b 1 SCSI scsi 0:1 => efidebug boot dump Boot0001: attributes: A-- (0x00000001) label: SCSI file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/ HD(1,GPT,0ed48d12-1b4c-4e08-b3ee-decf20428036,0x800,0xa000) data: 00000000: 00 00 => efideubg -b 2 USB usb 0:1 => efidebug boot order 2 1 => bootefi bootmgr Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- cmd/efidebug.c | 46 ++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 40 insertions(+), 6 deletions(-) diff --git a/cmd/efidebug.c b/cmd/efidebug.c index a977ca9c72f5..aaf269cdf47d 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -933,6 +933,29 @@ out: return initrd_dp; } +/** + * count_arguments - count the number of arguments + * @argc: Total number of arguments + * @argv: Argument array + * Return: Number of arguments + * + * Count the number of arguments for a given option, "-?" + * Specifically if the first argument is not "-?", return 0; + */ +static int count_arguments(int argc, char *const argv[]) +{ + int i; + + if (argv[0][0] != '-') + return 0; + + for (i = 1; i < argc; i++) + if (argv[i][0] == '-') + break; + + return i; +} + /** * do_efi_boot_add() - set UEFI load option * @@ -945,7 +968,7 @@ out: * * Implement efidebug "boot add" sub-command. Create or change UEFI load option. * - * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] <file> + * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] [<file>] * -i <file> <interface2> <devnum2>[:<part>] <initrd> * -s '<options>' */ @@ -979,7 +1002,10 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, argv++; /* 'add' */ for (; argc > 0; argc--, argv++) { if (!strcmp(argv[0], "-b")) { - if (argc < 5 || lo.label) { + int num_args; + + num_args = count_arguments(argc, argv); + if (num_args < 5 || num_args > 6 || lo.label) { r = CMD_RET_USAGE; goto out; } @@ -1000,7 +1026,8 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, utf8_utf16_strncpy(&label, argv[2], label_len); /* file path */ - ret = efi_dp_from_name(argv[3], argv[4], argv[5], + ret = efi_dp_from_name(argv[3], argv[4], + num_args == 5 ? "" : argv[5], &device_path, &file_path); if (ret != EFI_SUCCESS) { printf("Cannot create device path for \"%s %s\"\n", @@ -1008,10 +1035,16 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, r = CMD_RET_FAILURE; goto out; } + /* if no file name is given, use device_path */ + if (num_args == 5) { + efi_free_pool(file_path); + file_path = device_path; + device_path = NULL; + } fp_size += efi_dp_size(file_path) + sizeof(struct efi_device_path); - argc -= 5; - argv += 5; + argc -= num_args; + argv += num_args; } else if (!strcmp(argv[0], "-i")) { if (argc < 3 || initrd_dp) { r = CMD_RET_USAGE; @@ -1078,7 +1111,8 @@ out: free(data); efi_free_pool(final_fp); efi_free_pool(initrd_dp); - efi_free_pool(device_path); + if (device_path) + efi_free_pool(device_path); efi_free_pool(file_path); free(lo.label); -- 2.33.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [RFC 3/3] cmd: efidebug: handle booting from removable media 2021-11-09 1:32 ` [RFC 3/3] cmd: efidebug: handle " AKASHI Takahiro @ 2021-11-09 9:50 ` Heinrich Schuchardt 2021-11-10 6:24 ` AKASHI Takahiro 0 siblings, 1 reply; 13+ messages in thread From: Heinrich Schuchardt @ 2021-11-09 9:50 UTC (permalink / raw) To: AKASHI Takahiro, agraf; +Cc: sjg, ilias.apalodimas, u-boot On 11/9/21 02:32, AKASHI Takahiro wrote: > Support for booting from removable media is now added to UEFI boot > manager. Here we should modify efidebug command in order to define > a proper "BootXXXX" variable. > > With this patch applied, you will be able to specify the boot order, > usb and scsi: I guess for a removable device this should work even if the device is not present. But currently: => efidebug boot add -b 1000 USB_present usb 0:1 EFI/BOOT/BOOTARM.EFI => efidebug boot add -b 1000 USB_not_present usb 1:1 EFI/BOOT/BOOTARM.EFI Cannot create device path for "usb 1:1" A media device path like: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0)/HD(1,MBR,0x0c449046,0x800,0x800) is not very helpful because the next device that you insert may have a different location of the ESP partition. I think you should store /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0) and find the ESP on it at runtime. > => efidebug -b 1 SCSI scsi 0:1 > => efidebug boot dump > Boot0001: > attributes: A-- (0x00000001) > label: SCSI > file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/ > HD(1,GPT,0ed48d12-1b4c-4e08-b3ee-decf20428036,0x800,0xa000) > data: > 00000000: 00 00 > => efideubg -b 2 USB usb 0:1 efidebug Best regards Heinrich > => efidebug boot order 2 1 > => bootefi bootmgr > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > cmd/efidebug.c | 46 ++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 40 insertions(+), 6 deletions(-) > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > index a977ca9c72f5..aaf269cdf47d 100644 > --- a/cmd/efidebug.c > +++ b/cmd/efidebug.c > @@ -933,6 +933,29 @@ out: > return initrd_dp; > } > > +/** > + * count_arguments - count the number of arguments > + * @argc: Total number of arguments > + * @argv: Argument array > + * Return: Number of arguments > + * > + * Count the number of arguments for a given option, "-?" > + * Specifically if the first argument is not "-?", return 0; > + */ > +static int count_arguments(int argc, char *const argv[]) > +{ > + int i; > + > + if (argv[0][0] != '-') > + return 0; > + > + for (i = 1; i < argc; i++) > + if (argv[i][0] == '-') > + break; > + > + return i; > +} > + > /** > * do_efi_boot_add() - set UEFI load option > * > @@ -945,7 +968,7 @@ out: > * > * Implement efidebug "boot add" sub-command. Create or change UEFI load option. > * > - * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] <file> > + * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] [<file>] > * -i <file> <interface2> <devnum2>[:<part>] <initrd> > * -s '<options>' > */ > @@ -979,7 +1002,10 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > argv++; /* 'add' */ > for (; argc > 0; argc--, argv++) { > if (!strcmp(argv[0], "-b")) { > - if (argc < 5 || lo.label) { > + int num_args; > + > + num_args = count_arguments(argc, argv); > + if (num_args < 5 || num_args > 6 || lo.label) { > r = CMD_RET_USAGE; > goto out; > } > @@ -1000,7 +1026,8 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > utf8_utf16_strncpy(&label, argv[2], label_len); > > /* file path */ > - ret = efi_dp_from_name(argv[3], argv[4], argv[5], > + ret = efi_dp_from_name(argv[3], argv[4], > + num_args == 5 ? "" : argv[5], > &device_path, &file_path); > if (ret != EFI_SUCCESS) { > printf("Cannot create device path for \"%s %s\"\n", > @@ -1008,10 +1035,16 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > r = CMD_RET_FAILURE; > goto out; > } > + /* if no file name is given, use device_path */ > + if (num_args == 5) { > + efi_free_pool(file_path); > + file_path = device_path; > + device_path = NULL; > + } > fp_size += efi_dp_size(file_path) + > sizeof(struct efi_device_path); > - argc -= 5; > - argv += 5; > + argc -= num_args; > + argv += num_args; > } else if (!strcmp(argv[0], "-i")) { > if (argc < 3 || initrd_dp) { > r = CMD_RET_USAGE; > @@ -1078,7 +1111,8 @@ out: > free(data); > efi_free_pool(final_fp); > efi_free_pool(initrd_dp); > - efi_free_pool(device_path); > + if (device_path) > + efi_free_pool(device_path); > efi_free_pool(file_path); > free(lo.label); > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC 3/3] cmd: efidebug: handle booting from removable media 2021-11-09 9:50 ` Heinrich Schuchardt @ 2021-11-10 6:24 ` AKASHI Takahiro 2021-11-10 8:11 ` Heinrich Schuchardt 0 siblings, 1 reply; 13+ messages in thread From: AKASHI Takahiro @ 2021-11-10 6:24 UTC (permalink / raw) To: Heinrich Schuchardt; +Cc: agraf, sjg, ilias.apalodimas, u-boot On Tue, Nov 09, 2021 at 10:50:37AM +0100, Heinrich Schuchardt wrote: > On 11/9/21 02:32, AKASHI Takahiro wrote: > > Support for booting from removable media is now added to UEFI boot > > manager. Here we should modify efidebug command in order to define > > a proper "BootXXXX" variable. > > > > With this patch applied, you will be able to specify the boot order, > > usb and scsi: > > I guess for a removable device this should work even if the device is not > present. But currently: > > => efidebug boot add -b 1000 USB_present usb 0:1 EFI/BOOT/BOOTARM.EFI > => efidebug boot add -b 1000 USB_not_present usb 1:1 EFI/BOOT/BOOTARM.EFI > Cannot create device path for "usb 1:1" # In the second, I don't expect you to specify the file path, # "EFI/BOOT/BOOTARM.EFI", for removable media support. Yes, I have been aware of this issue but it is not this-patch specific but an existing issue as you clearly mentioned above. > A media device path like: > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0)/HD(1,MBR,0x0c449046,0x800,0x800) > > is not very helpful because the next device that you insert may have a > different location of the ESP partition. > > I think you should store > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0) Apparently it is promising but actually not because "UsbClass(0x781,0x5571,0x0,0x0,0x0)" contains Vendor ID and Product ID which can only be retrieved from a real device attached to the board. I'm not yet sure where "UsbClass(0x0,0x0,0x9,0x0,0x1)" comes from. "USB(X, Y)", where X is a parent_port_number and Y is a usb_interface, would be a more appropriate candidate for this kind of device path, but we don't utilise this form in the current implementation. > and find the ESP on it at runtime. I don't think the specification requires that the disk is an ESP (at least explicitly). > > => efidebug -b 1 SCSI scsi 0:1 > > => efidebug boot dump > > Boot0001: > > attributes: A-- (0x00000001) > > label: SCSI > > file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/ Contrary to USB, we use Scsi(0,0) for scsi devices but it is NOT identical to U-Boot's scsi0 (in boot_targets). > > HD(1,GPT,0ed48d12-1b4c-4e08-b3ee-decf20428036,0x800,0xa000) > > data: > > 00000000: 00 00 > > => efideubg -b 2 USB usb 0:1 > > efidebug OK -Takahiro Akashi > Best regards > > Heinrich > > > => efidebug boot order 2 1 > > => bootefi bootmgr > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > --- > > cmd/efidebug.c | 46 ++++++++++++++++++++++++++++++++++++++++------ > > 1 file changed, 40 insertions(+), 6 deletions(-) > > > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > > index a977ca9c72f5..aaf269cdf47d 100644 > > --- a/cmd/efidebug.c > > +++ b/cmd/efidebug.c > > @@ -933,6 +933,29 @@ out: > > return initrd_dp; > > } > > +/** > > + * count_arguments - count the number of arguments > > + * @argc: Total number of arguments > > + * @argv: Argument array > > + * Return: Number of arguments > > + * > > + * Count the number of arguments for a given option, "-?" > > + * Specifically if the first argument is not "-?", return 0; > > + */ > > +static int count_arguments(int argc, char *const argv[]) > > +{ > > + int i; > > + > > + if (argv[0][0] != '-') > > + return 0; > > + > > + for (i = 1; i < argc; i++) > > + if (argv[i][0] == '-') > > + break; > > + > > + return i; > > +} > > + > > /** > > * do_efi_boot_add() - set UEFI load option > > * > > @@ -945,7 +968,7 @@ out: > > * > > * Implement efidebug "boot add" sub-command. Create or change UEFI load option. > > * > > - * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] <file> > > + * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] [<file>] > > * -i <file> <interface2> <devnum2>[:<part>] <initrd> > > * -s '<options>' > > */ > > @@ -979,7 +1002,10 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > > argv++; /* 'add' */ > > for (; argc > 0; argc--, argv++) { > > if (!strcmp(argv[0], "-b")) { > > - if (argc < 5 || lo.label) { > > + int num_args; > > + > > + num_args = count_arguments(argc, argv); > > + if (num_args < 5 || num_args > 6 || lo.label) { > > r = CMD_RET_USAGE; > > goto out; > > } > > @@ -1000,7 +1026,8 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > > utf8_utf16_strncpy(&label, argv[2], label_len); > > /* file path */ > > - ret = efi_dp_from_name(argv[3], argv[4], argv[5], > > + ret = efi_dp_from_name(argv[3], argv[4], > > + num_args == 5 ? "" : argv[5], > > &device_path, &file_path); > > if (ret != EFI_SUCCESS) { > > printf("Cannot create device path for \"%s %s\"\n", > > @@ -1008,10 +1035,16 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, > > r = CMD_RET_FAILURE; > > goto out; > > } > > + /* if no file name is given, use device_path */ > > + if (num_args == 5) { > > + efi_free_pool(file_path); > > + file_path = device_path; > > + device_path = NULL; > > + } > > fp_size += efi_dp_size(file_path) + > > sizeof(struct efi_device_path); > > - argc -= 5; > > - argv += 5; > > + argc -= num_args; > > + argv += num_args; > > } else if (!strcmp(argv[0], "-i")) { > > if (argc < 3 || initrd_dp) { > > r = CMD_RET_USAGE; > > @@ -1078,7 +1111,8 @@ out: > > free(data); > > efi_free_pool(final_fp); > > efi_free_pool(initrd_dp); > > - efi_free_pool(device_path); > > + if (device_path) > > + efi_free_pool(device_path); > > efi_free_pool(file_path); > > free(lo.label); > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC 3/3] cmd: efidebug: handle booting from removable media 2021-11-10 6:24 ` AKASHI Takahiro @ 2021-11-10 8:11 ` Heinrich Schuchardt 2021-11-10 9:17 ` Heinrich Schuchardt 0 siblings, 1 reply; 13+ messages in thread From: Heinrich Schuchardt @ 2021-11-10 8:11 UTC (permalink / raw) To: AKASHI Takahiro; +Cc: Marek Vasut, agraf, sjg, ilias.apalodimas, u-boot On 11/10/21 07:24, AKASHI Takahiro wrote: > On Tue, Nov 09, 2021 at 10:50:37AM +0100, Heinrich Schuchardt wrote: >> On 11/9/21 02:32, AKASHI Takahiro wrote: >>> Support for booting from removable media is now added to UEFI boot >>> manager. Here we should modify efidebug command in order to define >>> a proper "BootXXXX" variable. >>> >>> With this patch applied, you will be able to specify the boot order, >>> usb and scsi: >> >> I guess for a removable device this should work even if the device is not >> present. But currently: >> >> => efidebug boot add -b 1000 USB_present usb 0:1 EFI/BOOT/BOOTARM.EFI >> => efidebug boot add -b 1000 USB_not_present usb 1:1 EFI/BOOT/BOOTARM.EFI >> Cannot create device path for "usb 1:1" > > # In the second, I don't expect you to specify the file path, > # "EFI/BOOT/BOOTARM.EFI", for removable media support. > > Yes, I have been aware of this issue but it is not this-patch specific > but an existing issue as you clearly mentioned above. > >> A media device path like: >> >> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0)/HD(1,MBR,0x0c449046,0x800,0x800) >> >> is not very helpful because the next device that you insert may have a >> different location of the ESP partition. >> >> I think you should store >> >> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0) > > Apparently it is promising but actually not because > "UsbClass(0x781,0x5571,0x0,0x0,0x0)" contains Vendor ID and > Product ID which can only be retrieved from a real device attached > to the board. 0x781,0x5571 relates to Vendor: SanDisk Corp. Device: Cruzer Fit This is how Linux sees my OrangePi PC: $ lsusb Bus 009 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub Bus 007 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 008 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub Bus 006 Device 002: ID 0781:5571 SanDisk Corp. Cruzer Fit Bus 006 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 005 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub The bus numbering seems not to be stable: $ lsusb Bus 009 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub Bus 008 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 007 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub Bus 005 Device 002: ID 0781:5571 SanDisk Corp. Cruzer Fit Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 006 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub And this is in U-Boot: usb 0 [ + ] ehci_generic | |-- usb@1c1a000 usb_hub 0 [ + ] usb_hub | | `-- usb_hub usb 1 [ + ] ohci_generic | |-- usb@1c1a400 usb_hub 1 [ + ] usb_hub | | `-- usb_hub usb 2 [ + ] ehci_generic | |-- usb@1c1b000 usb_hub 2 [ + ] usb_hub | | `-- usb_hub usb 3 [ + ] ohci_generic | |-- usb@1c1b400 usb_hub 3 [ + ] usb_hub | | `-- usb_hub usb 4 [ + ] ehci_generic | |-- usb@1c1c000 usb_hub 4 [ + ] usb_hub | | `-- usb_hub usb_mass_s 0 [ + ] usb_mass_storage | | `-- usb_mass_storage blk 1 [ ] usb_storage_blk | | `-- usb_mass_storage.lun0 usb 5 [ + ] ohci_generic | |-- usb@1c1c400 usb_hub 5 [ + ] usb_hub | | `-- usb_hub usb 6 [ + ] ehci_generic | |-- usb@1c1d000 usb_hub 6 [ + ] usb_hub | | `-- usb_hub usb 7 [ + ] ohci_generic | |-- usb@1c1d400 usb_hub 7 [ + ] usb_hub | | `-- usb_hub The location where a USB is plugged is identified by the port of the USB hub it is connected to. I think before we can properly support removable media we will have to get the UEFI device path right. We need a node on all of the levels of the DM tree of which more than one instance can exist. Best regards Heinrich > > I'm not yet sure where "UsbClass(0x0,0x0,0x9,0x0,0x1)" comes from. > > "USB(X, Y)", where X is a parent_port_number and Y is a usb_interface, > would be a more appropriate candidate for this kind of device path, > but we don't utilise this form in the current implementation. > >> and find the ESP on it at runtime. > > I don't think the specification requires that the disk is an ESP > (at least explicitly). > > >>> => efidebug -b 1 SCSI scsi 0:1 >>> => efidebug boot dump >>> Boot0001: >>> attributes: A-- (0x00000001) >>> label: SCSI >>> file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/ > > Contrary to USB, we use Scsi(0,0) for scsi devices but it is NOT identical > to U-Boot's scsi0 (in boot_targets). > >>> HD(1,GPT,0ed48d12-1b4c-4e08-b3ee-decf20428036,0x800,0xa000) >>> data: >>> 00000000: 00 00 >>> => efideubg -b 2 USB usb 0:1 >> >> efidebug > > OK > > -Takahiro Akashi > >> Best regards >> >> Heinrich >> >>> => efidebug boot order 2 1 >>> => bootefi bootmgr >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>> --- >>> cmd/efidebug.c | 46 ++++++++++++++++++++++++++++++++++++++++------ >>> 1 file changed, 40 insertions(+), 6 deletions(-) >>> >>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c >>> index a977ca9c72f5..aaf269cdf47d 100644 >>> --- a/cmd/efidebug.c >>> +++ b/cmd/efidebug.c >>> @@ -933,6 +933,29 @@ out: >>> return initrd_dp; >>> } >>> +/** >>> + * count_arguments - count the number of arguments >>> + * @argc: Total number of arguments >>> + * @argv: Argument array >>> + * Return: Number of arguments >>> + * >>> + * Count the number of arguments for a given option, "-?" >>> + * Specifically if the first argument is not "-?", return 0; >>> + */ >>> +static int count_arguments(int argc, char *const argv[]) >>> +{ >>> + int i; >>> + >>> + if (argv[0][0] != '-') >>> + return 0; >>> + >>> + for (i = 1; i < argc; i++) >>> + if (argv[i][0] == '-') >>> + break; >>> + >>> + return i; >>> +} >>> + >>> /** >>> * do_efi_boot_add() - set UEFI load option >>> * >>> @@ -945,7 +968,7 @@ out: >>> * >>> * Implement efidebug "boot add" sub-command. Create or change UEFI load option. >>> * >>> - * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] <file> >>> + * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] [<file>] >>> * -i <file> <interface2> <devnum2>[:<part>] <initrd> >>> * -s '<options>' >>> */ >>> @@ -979,7 +1002,10 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, >>> argv++; /* 'add' */ >>> for (; argc > 0; argc--, argv++) { >>> if (!strcmp(argv[0], "-b")) { >>> - if (argc < 5 || lo.label) { >>> + int num_args; >>> + >>> + num_args = count_arguments(argc, argv); >>> + if (num_args < 5 || num_args > 6 || lo.label) { >>> r = CMD_RET_USAGE; >>> goto out; >>> } >>> @@ -1000,7 +1026,8 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, >>> utf8_utf16_strncpy(&label, argv[2], label_len); >>> /* file path */ >>> - ret = efi_dp_from_name(argv[3], argv[4], argv[5], >>> + ret = efi_dp_from_name(argv[3], argv[4], >>> + num_args == 5 ? "" : argv[5], >>> &device_path, &file_path); >>> if (ret != EFI_SUCCESS) { >>> printf("Cannot create device path for \"%s %s\"\n", >>> @@ -1008,10 +1035,16 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, >>> r = CMD_RET_FAILURE; >>> goto out; >>> } >>> + /* if no file name is given, use device_path */ >>> + if (num_args == 5) { >>> + efi_free_pool(file_path); >>> + file_path = device_path; >>> + device_path = NULL; >>> + } >>> fp_size += efi_dp_size(file_path) + >>> sizeof(struct efi_device_path); >>> - argc -= 5; >>> - argv += 5; >>> + argc -= num_args; >>> + argv += num_args; >>> } else if (!strcmp(argv[0], "-i")) { >>> if (argc < 3 || initrd_dp) { >>> r = CMD_RET_USAGE; >>> @@ -1078,7 +1111,8 @@ out: >>> free(data); >>> efi_free_pool(final_fp); >>> efi_free_pool(initrd_dp); >>> - efi_free_pool(device_path); >>> + if (device_path) >>> + efi_free_pool(device_path); >>> efi_free_pool(file_path); >>> free(lo.label); >>> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC 3/3] cmd: efidebug: handle booting from removable media 2021-11-10 8:11 ` Heinrich Schuchardt @ 2021-11-10 9:17 ` Heinrich Schuchardt 2021-11-12 1:51 ` AKASHI Takahiro 0 siblings, 1 reply; 13+ messages in thread From: Heinrich Schuchardt @ 2021-11-10 9:17 UTC (permalink / raw) To: AKASHI Takahiro; +Cc: Marek Vasut, agraf, sjg, ilias.apalodimas, u-boot On 11/10/21 09:11, Heinrich Schuchardt wrote: > > > On 11/10/21 07:24, AKASHI Takahiro wrote: >> On Tue, Nov 09, 2021 at 10:50:37AM +0100, Heinrich Schuchardt wrote: >>> On 11/9/21 02:32, AKASHI Takahiro wrote: >>>> Support for booting from removable media is now added to UEFI boot >>>> manager. Here we should modify efidebug command in order to define >>>> a proper "BootXXXX" variable. >>>> >>>> With this patch applied, you will be able to specify the boot order, >>>> usb and scsi: >>> >>> I guess for a removable device this should work even if the device is >>> not >>> present. But currently: >>> >>> => efidebug boot add -b 1000 USB_present usb 0:1 EFI/BOOT/BOOTARM.EFI >>> => efidebug boot add -b 1000 USB_not_present usb 1:1 >>> EFI/BOOT/BOOTARM.EFI >>> Cannot create device path for "usb 1:1" >> >> # In the second, I don't expect you to specify the file path, >> # "EFI/BOOT/BOOTARM.EFI", for removable media support. >> >> Yes, I have been aware of this issue but it is not this-patch specific >> but an existing issue as you clearly mentioned above. >> >>> A media device path like: >>> >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0)/HD(1,MBR,0x0c449046,0x800,0x800) >>> >>> >>> is not very helpful because the next device that you insert may have a >>> different location of the ESP partition. >>> >>> I think you should store >>> >>> /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0) >>> >> >> Apparently it is promising but actually not because >> "UsbClass(0x781,0x5571,0x0,0x0,0x0)" contains Vendor ID and >> Product ID which can only be retrieved from a real device attached >> to the board. > > 0x781,0x5571 relates to > Vendor: SanDisk Corp. > Device: Cruzer Fit > > This is how Linux sees my OrangePi PC: > > $ lsusb > Bus 009 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > Bus 007 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > Bus 008 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > Bus 006 Device 002: ID 0781:5571 SanDisk Corp. Cruzer Fit > Bus 006 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > Bus 005 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > The bus numbering seems not to be stable: > > $ lsusb > Bus 009 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > Bus 008 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > Bus 007 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > Bus 005 Device 002: ID 0781:5571 SanDisk Corp. Cruzer Fit > Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > Bus 006 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > > And this is in U-Boot: > > usb 0 [ + ] ehci_generic | |-- usb@1c1a000 > usb_hub 0 [ + ] usb_hub | | `-- usb_hub > usb 1 [ + ] ohci_generic | |-- usb@1c1a400 > usb_hub 1 [ + ] usb_hub | | `-- usb_hub > usb 2 [ + ] ehci_generic | |-- usb@1c1b000 > usb_hub 2 [ + ] usb_hub | | `-- usb_hub > usb 3 [ + ] ohci_generic | |-- usb@1c1b400 > usb_hub 3 [ + ] usb_hub | | `-- usb_hub > usb 4 [ + ] ehci_generic | |-- usb@1c1c000 > usb_hub 4 [ + ] usb_hub | | `-- usb_hub > usb_mass_s 0 [ + ] usb_mass_storage | | `-- > usb_mass_storage > blk 1 [ ] usb_storage_blk | | `-- > usb_mass_storage.lun0 > usb 5 [ + ] ohci_generic | |-- usb@1c1c400 > usb_hub 5 [ + ] usb_hub | | `-- usb_hub > usb 6 [ + ] ehci_generic | |-- usb@1c1d000 > usb_hub 6 [ + ] usb_hub | | `-- usb_hub > usb 7 [ + ] ohci_generic | |-- usb@1c1d400 > usb_hub 7 [ + ] usb_hub | | `-- usb_hub > > The location where a USB is plugged is identified by the port of the USB > hub it is connected to. > > I think before we can properly support removable media we will have to > get the UEFI device path right. We need a node on all of the levels of > the DM tree of which more than one instance can exist. Here is an example of device path created by an AMI BIOS for an ESP partition on a USB stick: PciRoot(0x0)/Pci(0x1,0x2)/Pci(0x0,0x0)/Pci(0x8,0x0)/Pci(0x0,0x1)/USB(0x2,0x0)/USB(0x3,0x0)/HD(1,GPT,1AD3DE75-504A-47DF-A5CE-81D9EF0252A2,0x40,0x55D824) The two number in PCI(foo, bar) are: PCI device PCI function The two numbers in USB(foo, bar) are: USB Parent Port Number USB Interface Number By using USB() instead of USBClass() the device path becomes generic. If you plug in a device from a different vendor into the same USB port, you will get the same device path for the device. Only the partition information may change. Best regards Heinrich > >> >> I'm not yet sure where "UsbClass(0x0,0x0,0x9,0x0,0x1)" comes from. >> >> "USB(X, Y)", where X is a parent_port_number and Y is a usb_interface, >> would be a more appropriate candidate for this kind of device path, >> but we don't utilise this form in the current implementation. >> >>> and find the ESP on it at runtime. >> >> I don't think the specification requires that the disk is an ESP >> (at least explicitly). >> >> >>>> => efidebug -b 1 SCSI scsi 0:1 >>>> => efidebug boot dump >>>> Boot0001: >>>> attributes: A-- (0x00000001) >>>> label: SCSI >>>> file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/ >> >> Contrary to USB, we use Scsi(0,0) for scsi devices but it is NOT >> identical >> to U-Boot's scsi0 (in boot_targets). >> >>>> HD(1,GPT,0ed48d12-1b4c-4e08-b3ee-decf20428036,0x800,0xa000) >>>> data: >>>> 00000000: 00 00 >>>> => efideubg -b 2 USB usb 0:1 >>> >>> efidebug >> >> OK >> >> -Takahiro Akashi >> >>> Best regards >>> >>> Heinrich >>> >>>> => efidebug boot order 2 1 >>>> => bootefi bootmgr >>>> >>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >>>> --- >>>> cmd/efidebug.c | 46 ++++++++++++++++++++++++++++++++++++++++------ >>>> 1 file changed, 40 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/cmd/efidebug.c b/cmd/efidebug.c >>>> index a977ca9c72f5..aaf269cdf47d 100644 >>>> --- a/cmd/efidebug.c >>>> +++ b/cmd/efidebug.c >>>> @@ -933,6 +933,29 @@ out: >>>> return initrd_dp; >>>> } >>>> +/** >>>> + * count_arguments - count the number of arguments >>>> + * @argc: Total number of arguments >>>> + * @argv: Argument array >>>> + * Return: Number of arguments >>>> + * >>>> + * Count the number of arguments for a given option, "-?" >>>> + * Specifically if the first argument is not "-?", return 0; >>>> + */ >>>> +static int count_arguments(int argc, char *const argv[]) >>>> +{ >>>> + int i; >>>> + >>>> + if (argv[0][0] != '-') >>>> + return 0; >>>> + >>>> + for (i = 1; i < argc; i++) >>>> + if (argv[i][0] == '-') >>>> + break; >>>> + >>>> + return i; >>>> +} >>>> + >>>> /** >>>> * do_efi_boot_add() - set UEFI load option >>>> * >>>> @@ -945,7 +968,7 @@ out: >>>> * >>>> * Implement efidebug "boot add" sub-command. Create or change >>>> UEFI load option. >>>> * >>>> - * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] >>>> <file> >>>> + * efidebug boot add -b <id> <label> <interface> <devnum>[:<part>] >>>> [<file>] >>>> * -i <file> <interface2> <devnum2>[:<part>] >>>> <initrd> >>>> * -s '<options>' >>>> */ >>>> @@ -979,7 +1002,10 @@ static int do_efi_boot_add(struct cmd_tbl >>>> *cmdtp, int flag, >>>> argv++; /* 'add' */ >>>> for (; argc > 0; argc--, argv++) { >>>> if (!strcmp(argv[0], "-b")) { >>>> - if (argc < 5 || lo.label) { >>>> + int num_args; >>>> + >>>> + num_args = count_arguments(argc, argv); >>>> + if (num_args < 5 || num_args > 6 || lo.label) { >>>> r = CMD_RET_USAGE; >>>> goto out; >>>> } >>>> @@ -1000,7 +1026,8 @@ static int do_efi_boot_add(struct cmd_tbl >>>> *cmdtp, int flag, >>>> utf8_utf16_strncpy(&label, argv[2], label_len); >>>> /* file path */ >>>> - ret = efi_dp_from_name(argv[3], argv[4], argv[5], >>>> + ret = efi_dp_from_name(argv[3], argv[4], >>>> + num_args == 5 ? "" : argv[5], >>>> &device_path, &file_path); >>>> if (ret != EFI_SUCCESS) { >>>> printf("Cannot create device path for \"%s %s\"\n", >>>> @@ -1008,10 +1035,16 @@ static int do_efi_boot_add(struct cmd_tbl >>>> *cmdtp, int flag, >>>> r = CMD_RET_FAILURE; >>>> goto out; >>>> } >>>> + /* if no file name is given, use device_path */ >>>> + if (num_args == 5) { >>>> + efi_free_pool(file_path); >>>> + file_path = device_path; >>>> + device_path = NULL; >>>> + } >>>> fp_size += efi_dp_size(file_path) + >>>> sizeof(struct efi_device_path); >>>> - argc -= 5; >>>> - argv += 5; >>>> + argc -= num_args; >>>> + argv += num_args; >>>> } else if (!strcmp(argv[0], "-i")) { >>>> if (argc < 3 || initrd_dp) { >>>> r = CMD_RET_USAGE; >>>> @@ -1078,7 +1111,8 @@ out: >>>> free(data); >>>> efi_free_pool(final_fp); >>>> efi_free_pool(initrd_dp); >>>> - efi_free_pool(device_path); >>>> + if (device_path) >>>> + efi_free_pool(device_path); >>>> efi_free_pool(file_path); >>>> free(lo.label); >>>> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC 3/3] cmd: efidebug: handle booting from removable media 2021-11-10 9:17 ` Heinrich Schuchardt @ 2021-11-12 1:51 ` AKASHI Takahiro 0 siblings, 0 replies; 13+ messages in thread From: AKASHI Takahiro @ 2021-11-12 1:51 UTC (permalink / raw) To: Heinrich Schuchardt; +Cc: Marek Vasut, agraf, sjg, ilias.apalodimas, u-boot On Wed, Nov 10, 2021 at 10:17:50AM +0100, Heinrich Schuchardt wrote: > On 11/10/21 09:11, Heinrich Schuchardt wrote: > > > > > > On 11/10/21 07:24, AKASHI Takahiro wrote: > > > On Tue, Nov 09, 2021 at 10:50:37AM +0100, Heinrich Schuchardt wrote: > > > > On 11/9/21 02:32, AKASHI Takahiro wrote: > > > > > Support for booting from removable media is now added to UEFI boot > > > > > manager. Here we should modify efidebug command in order to define > > > > > a proper "BootXXXX" variable. > > > > > > > > > > With this patch applied, you will be able to specify the boot order, > > > > > usb and scsi: > > > > > > > > I guess for a removable device this should work even if the > > > > device is not > > > > present. But currently: > > > > > > > > => efidebug boot add -b 1000 USB_present usb 0:1 EFI/BOOT/BOOTARM.EFI > > > > => efidebug boot add -b 1000 USB_not_present usb 1:1 > > > > EFI/BOOT/BOOTARM.EFI > > > > Cannot create device path for "usb 1:1" > > > > > > # In the second, I don't expect you to specify the file path, > > > # "EFI/BOOT/BOOTARM.EFI", for removable media support. > > > > > > Yes, I have been aware of this issue but it is not this-patch specific > > > but an existing issue as you clearly mentioned above. > > > > > > > A media device path like: > > > > > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0)/HD(1,MBR,0x0c449046,0x800,0x800) > > > > > > > > > > > > is not very helpful because the next device that you insert may have a > > > > different location of the ESP partition. > > > > > > > > I think you should store > > > > > > > > /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/UsbClass(0x0,0x0,0x9,0x0,0x1)/UsbClass(0x781,0x5571,0x0,0x0,0x0) > > > > > > > > > > Apparently it is promising but actually not because > > > "UsbClass(0x781,0x5571,0x0,0x0,0x0)" contains Vendor ID and > > > Product ID which can only be retrieved from a real device attached > > > to the board. > > > > 0x781,0x5571 relates to > > Vendor: SanDisk Corp. > > Device: Cruzer Fit > > > > This is how Linux sees my OrangePi PC: > > > > $ lsusb > > Bus 009 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > > Bus 007 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > Bus 008 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > > Bus 006 Device 002: ID 0781:5571 SanDisk Corp. Cruzer Fit > > Bus 006 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > Bus 005 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > > Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > > Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > > > The bus numbering seems not to be stable: > > > > $ lsusb > > Bus 009 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > > Bus 008 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > Bus 007 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > > Bus 005 Device 002: ID 0781:5571 SanDisk Corp. Cruzer Fit > > Bus 005 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > Bus 006 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > > Bus 004 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > Bus 003 Device 001: ID 1d6b:0001 Linux Foundation 1.1 root hub > > Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > Bus 002 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub > > > > > > And this is in U-Boot: > > > > usb 0 [ + ] ehci_generic | |-- usb@1c1a000 > > usb_hub 0 [ + ] usb_hub | | `-- usb_hub > > usb 1 [ + ] ohci_generic | |-- usb@1c1a400 > > usb_hub 1 [ + ] usb_hub | | `-- usb_hub > > usb 2 [ + ] ehci_generic | |-- usb@1c1b000 > > usb_hub 2 [ + ] usb_hub | | `-- usb_hub > > usb 3 [ + ] ohci_generic | |-- usb@1c1b400 > > usb_hub 3 [ + ] usb_hub | | `-- usb_hub > > usb 4 [ + ] ehci_generic | |-- usb@1c1c000 > > usb_hub 4 [ + ] usb_hub | | `-- usb_hub > > usb_mass_s 0 [ + ] usb_mass_storage | > > | `-- usb_mass_storage > > blk 1 [ ] usb_storage_blk > > | | `-- usb_mass_storage.lun0 > > usb 5 [ + ] ohci_generic | |-- usb@1c1c400 > > usb_hub 5 [ + ] usb_hub | | `-- usb_hub > > usb 6 [ + ] ehci_generic | |-- usb@1c1d000 > > usb_hub 6 [ + ] usb_hub | | `-- usb_hub > > usb 7 [ + ] ohci_generic | |-- usb@1c1d400 > > usb_hub 7 [ + ] usb_hub | | `-- usb_hub > > > > The location where a USB is plugged is identified by the port of the USB > > hub it is connected to. > > > > I think before we can properly support removable media we will have to > > get the UEFI device path right. We need a node on all of the levels of > > the DM tree of which more than one instance can exist. > > Here is an example of device path created by an AMI BIOS for an ESP > partition on a USB stick: > > PciRoot(0x0)/Pci(0x1,0x2)/Pci(0x0,0x0)/Pci(0x8,0x0)/Pci(0x0,0x1)/USB(0x2,0x0)/USB(0x3,0x0)/HD(1,GPT,1AD3DE75-504A-47DF-A5CE-81D9EF0252A2,0x40,0x55D824) > > The two number in PCI(foo, bar) are: > PCI device > PCI function > > The two numbers in USB(foo, bar) are: > USB Parent Port Number > USB Interface Number > > By using USB() instead of USBClass() the device path becomes generic. If you > plug in a device from a different vendor into the same USB port, you will > get the same device path for the device. Only the partition information may > change. I agree here as it is what I have mentioned in my reply. My concern is that we may not be able to create such a detailed device path from DM representation. For example, "usb 0:1" doesn't contain any information on usb ports or interfaces. -Takahiro Akashi > Best regards > > Heinrich > > > > > > > > > I'm not yet sure where "UsbClass(0x0,0x0,0x9,0x0,0x1)" comes from. > > > > > > "USB(X, Y)", where X is a parent_port_number and Y is a usb_interface, > > > would be a more appropriate candidate for this kind of device path, > > > but we don't utilise this form in the current implementation. > > > > > > > and find the ESP on it at runtime. > > > > > > I don't think the specification requires that the disk is an ESP > > > (at least explicitly). > > > > > > > > > > > => efidebug -b 1 SCSI scsi 0:1 > > > > > => efidebug boot dump > > > > > Boot0001: > > > > > attributes: A-- (0x00000001) > > > > > label: SCSI > > > > > file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/Scsi(0,0)/ > > > > > > Contrary to USB, we use Scsi(0,0) for scsi devices but it is NOT > > > identical > > > to U-Boot's scsi0 (in boot_targets). > > > > > > > > HD(1,GPT,0ed48d12-1b4c-4e08-b3ee-decf20428036,0x800,0xa000) > > > > > data: > > > > > 00000000: 00 00 > > > > > => efideubg -b 2 USB usb 0:1 > > > > > > > > efidebug > > > > > > OK > > > > > > -Takahiro Akashi > > > > > > > Best regards > > > > > > > > Heinrich > > > > > > > > > => efidebug boot order 2 1 > > > > > => bootefi bootmgr > > > > > > > > > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > > > > --- > > > > > cmd/efidebug.c | 46 ++++++++++++++++++++++++++++++++++++++++------ > > > > > 1 file changed, 40 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > > > > > index a977ca9c72f5..aaf269cdf47d 100644 > > > > > --- a/cmd/efidebug.c > > > > > +++ b/cmd/efidebug.c > > > > > @@ -933,6 +933,29 @@ out: > > > > > return initrd_dp; > > > > > } > > > > > +/** > > > > > + * count_arguments - count the number of arguments > > > > > + * @argc: Total number of arguments > > > > > + * @argv: Argument array > > > > > + * Return: Number of arguments > > > > > + * > > > > > + * Count the number of arguments for a given option, "-?" > > > > > + * Specifically if the first argument is not "-?", return 0; > > > > > + */ > > > > > +static int count_arguments(int argc, char *const argv[]) > > > > > +{ > > > > > + int i; > > > > > + > > > > > + if (argv[0][0] != '-') > > > > > + return 0; > > > > > + > > > > > + for (i = 1; i < argc; i++) > > > > > + if (argv[i][0] == '-') > > > > > + break; > > > > > + > > > > > + return i; > > > > > +} > > > > > + > > > > > /** > > > > > * do_efi_boot_add() - set UEFI load option > > > > > * > > > > > @@ -945,7 +968,7 @@ out: > > > > > * > > > > > * Implement efidebug "boot add" sub-command. Create > > > > > or change UEFI load option. > > > > > * > > > > > - * efidebug boot add -b <id> <label> <interface> > > > > > <devnum>[:<part>] <file> > > > > > + * efidebug boot add -b <id> <label> <interface> > > > > > <devnum>[:<part>] [<file>] > > > > > * -i <file> > > > > > <interface2> <devnum2>[:<part>] <initrd> > > > > > * -s '<options>' > > > > > */ > > > > > @@ -979,7 +1002,10 @@ static int do_efi_boot_add(struct > > > > > cmd_tbl *cmdtp, int flag, > > > > > argv++; /* 'add' */ > > > > > for (; argc > 0; argc--, argv++) { > > > > > if (!strcmp(argv[0], "-b")) { > > > > > - if (argc < 5 || lo.label) { > > > > > + int num_args; > > > > > + > > > > > + num_args = count_arguments(argc, argv); > > > > > + if (num_args < 5 || num_args > 6 || lo.label) { > > > > > r = CMD_RET_USAGE; > > > > > goto out; > > > > > } > > > > > @@ -1000,7 +1026,8 @@ static int do_efi_boot_add(struct > > > > > cmd_tbl *cmdtp, int flag, > > > > > utf8_utf16_strncpy(&label, argv[2], label_len); > > > > > /* file path */ > > > > > - ret = efi_dp_from_name(argv[3], argv[4], argv[5], > > > > > + ret = efi_dp_from_name(argv[3], argv[4], > > > > > + num_args == 5 ? "" : argv[5], > > > > > &device_path, &file_path); > > > > > if (ret != EFI_SUCCESS) { > > > > > printf("Cannot create device path for \"%s %s\"\n", > > > > > @@ -1008,10 +1035,16 @@ static int do_efi_boot_add(struct > > > > > cmd_tbl *cmdtp, int flag, > > > > > r = CMD_RET_FAILURE; > > > > > goto out; > > > > > } > > > > > + /* if no file name is given, use device_path */ > > > > > + if (num_args == 5) { > > > > > + efi_free_pool(file_path); > > > > > + file_path = device_path; > > > > > + device_path = NULL; > > > > > + } > > > > > fp_size += efi_dp_size(file_path) + > > > > > sizeof(struct efi_device_path); > > > > > - argc -= 5; > > > > > - argv += 5; > > > > > + argc -= num_args; > > > > > + argv += num_args; > > > > > } else if (!strcmp(argv[0], "-i")) { > > > > > if (argc < 3 || initrd_dp) { > > > > > r = CMD_RET_USAGE; > > > > > @@ -1078,7 +1111,8 @@ out: > > > > > free(data); > > > > > efi_free_pool(final_fp); > > > > > efi_free_pool(initrd_dp); > > > > > - efi_free_pool(device_path); > > > > > + if (device_path) > > > > > + efi_free_pool(device_path); > > > > > efi_free_pool(file_path); > > > > > free(lo.label); > > > > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-11-12 1:51 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-09 1:32 [RFC 0/3] efi_loader: bootmgr itself should support removable media AKASHI Takahiro 2021-11-09 1:32 ` [RFC 1/3] efi_loader: export efi_locate_device_handle() AKASHI Takahiro 2021-11-09 1:32 ` [RFC 2/3] efi_loader: bootmgr: add booting from removable media AKASHI Takahiro 2021-11-09 9:14 ` Heinrich Schuchardt 2021-11-10 5:38 ` AKASHI Takahiro 2021-11-09 13:53 ` Mark Kettenis 2021-11-10 5:52 ` AKASHI Takahiro 2021-11-09 1:32 ` [RFC 3/3] cmd: efidebug: handle " AKASHI Takahiro 2021-11-09 9:50 ` Heinrich Schuchardt 2021-11-10 6:24 ` AKASHI Takahiro 2021-11-10 8:11 ` Heinrich Schuchardt 2021-11-10 9:17 ` Heinrich Schuchardt 2021-11-12 1:51 ` AKASHI Takahiro
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.