[RFC 0/3] efi_loader: bootmgr itself should support removable media

[RFC 0/3] efi_loader: bootmgr itself should support removable media

[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

[RFC 1/3] efi_loader: export efi_locate_device_handle()

[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

[RFC 2/3] efi_loader: bootmgr: add booting from removable media

[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

Re: [RFC 2/3] efi_loader: bootmgr: add booting from removable media

[Heinrich Schuchardt]

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

Re: [RFC 2/3] efi_loader: bootmgr: add booting from removable media

[AKASHI Takahiro]

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

Re: [RFC 2/3] efi_loader: bootmgr: add booting from removable media

[Mark Kettenis]

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

Re: [RFC 2/3] efi_loader: bootmgr: add booting from removable media

[AKASHI Takahiro]

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

[RFC 3/3] cmd: efidebug: handle booting from removable media

[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

Re: [RFC 3/3] cmd: efidebug: handle booting from removable media

[Heinrich Schuchardt]

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

Re: [RFC 3/3] cmd: efidebug: handle booting from removable media

[AKASHI Takahiro]

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

Re: [RFC 3/3] cmd: efidebug: handle booting from removable media

[Heinrich Schuchardt]

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

Re: [RFC 3/3] cmd: efidebug: handle booting from removable media

[Heinrich Schuchardt]

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

Re: [RFC 3/3] cmd: efidebug: handle booting from removable media

[AKASHI Takahiro]

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