diff mbox series

[v4,06/11] efi_loader: bootmgr: add booting from removable media

Message ID 20220324135443.1571-7-masahisa.kojima@linaro.org
State Superseded
Headers show
Series enable menu-driven boot device selection | expand

Commit Message

Masahisa Kojima March 24, 2022, 1:54 p.m. UTC
From: AKASHI Takahiro <takahiro.akashi@linaro.org>

Under the current implementation, booting from removable 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>
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
Changes from original version:
- create new include file "efi_default_filename.h" to
  avoid conflict with config_distro_bootcmd.h
- modify the target pointer of efi_free_pool(), expand_media_path() should
  only free the pointer allocated by efi_dp_from_file() function.

 include/config_distro_bootcmd.h | 14 +--------
 include/efi_default_filename.h  | 26 +++++++++++++++++
 lib/efi_loader/efi_bootmgr.c    | 50 ++++++++++++++++++++++++++++++++-
 3 files changed, 76 insertions(+), 14 deletions(-)
 create mode 100644 include/efi_default_filename.h

Comments

Ilias Apalodimas March 30, 2022, 7:13 p.m. UTC | #1
Hello Akashi-san,

On Thu, Mar 24, 2022 at 10:54:38PM +0900, Masahisa Kojima wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> Under the current implementation, booting from removable 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

Mark had some concerns wrt to this approach and from what I can tell this
hasn't changed in this revision [1]. Can we use boot_targets and generate
Boot#### with an empty FilePath as Mark suggested?  Or is the user expected to
select that somehow from the menu?

[1] https://lore.kernel.org/u-boot/d3cac2e5b37f96b5@bloch.sibelius.xs4all.nl/

Thanks
/Ilias
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes from original version:
> - create new include file "efi_default_filename.h" to
>   avoid conflict with config_distro_bootcmd.h
> - modify the target pointer of efi_free_pool(), expand_media_path() should
>   only free the pointer allocated by efi_dp_from_file() function.
> 
>  include/config_distro_bootcmd.h | 14 +--------
>  include/efi_default_filename.h  | 26 +++++++++++++++++
>  lib/efi_loader/efi_bootmgr.c    | 50 ++++++++++++++++++++++++++++++++-
>  3 files changed, 76 insertions(+), 14 deletions(-)
>  create mode 100644 include/efi_default_filename.h
> 
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index 2f90929178..ef2c9f330e 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -91,19 +91,7 @@
>  #endif
>  
>  #ifdef CONFIG_EFI_LOADER
> -#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"
> -#endif
> +#include <efi_default_filename.h>
>  #endif
>  
>  #ifdef BOOTEFI_NAME
> diff --git a/include/efi_default_filename.h b/include/efi_default_filename.h
> new file mode 100644
> index 0000000000..de030d2692
> --- /dev/null
> +++ b/include/efi_default_filename.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Default boot file name when none is present in the FilePath.
> + *
> + * Copyright (c) 2022, Linaro Limited
> + */
> +#ifndef _EFI_DEFAULT_FILENAME_H
> +#define _EFI_DEFAULT_FILENAME_H
> +
> +#if defined(CONFIG_ARM64)
> +#define BOOTEFI_NAME "BOOTAA64.EFI"
> +#elif defined(CONFIG_ARM)
> +#define BOOTEFI_NAME "BOOTARM.EFI"
> +#elif defined(CONFIG_X86_64)
> +#define BOOTEFI_NAME "BOOTX64.EFI"
> +#elif defined(CONFIG_X86)
> +#define BOOTEFI_NAME "BOOTIA32.EFI"
> +#elif defined(CONFIG_ARCH_RV32I)
> +#define BOOTEFI_NAME "BOOTRISCV32.EFI"
> +#elif defined(CONFIG_ARCH_RV64I)
> +#define BOOTEFI_NAME "BOOTRISCV64.EFI"
> +#else
> +#error Unsupported UEFI architecture
> +#endif
> +
> +#endif
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 8c04ecbdc8..22a4302aac 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -11,6 +11,7 @@
>  #include <charset.h>
>  #include <log.h>
>  #include <malloc.h>
> +#include <efi_default_filename.h>
>  #include <efi_loader.h>
>  #include <efi_variable.h>
>  #include <asm/unaligned.h>
> @@ -30,6 +31,50 @@ static const struct efi_runtime_services *rs;
>   * should do normal or recovery boot.
>   */
>  
> +/**
> + * 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);
> +			efi_free_pool(dp);
> +		} else {
> +			full_path = efi_dp_dup(device_path);
> +		}
> +	} else {
> +		full_path = efi_dp_dup(device_path);
> +	}
> +
> +	return full_path;
> +}
> +
>  /**
>   * try_load_entry() - try to load image for boot option
>   *
> @@ -68,13 +113,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.17.1
>
Masahisa Kojima March 31, 2022, 12:51 a.m. UTC | #2
Hi Ilias,

On Thu, 31 Mar 2022 at 04:13, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hello Akashi-san,
>
> On Thu, Mar 24, 2022 at 10:54:38PM +0900, Masahisa Kojima wrote:
> > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >
> > Under the current implementation, booting from removable 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
>
> Mark had some concerns wrt to this approach and from what I can tell this
> hasn't changed in this revision [1]. Can we use boot_targets and generate
> Boot#### with an empty FilePath as Mark suggested?  Or is the user expected to
> select that somehow from the menu?
>
> [1] https://lore.kernel.org/u-boot/d3cac2e5b37f96b5@bloch.sibelius.xs4all.nl/

My patch series tries to resolve the above issue to some extent
by a different approach.

The patch "[PATCH v4 10/11] bootmenu: add removable media entries" [*1]
enumerates the all (removable) medias supporting
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.

Example bootmenu is as below.

    *** U-Boot Boot Menu ***

       UEFI BOOT0000 : mmc0:1
       UEFI BOOT0001 : mmc0:2
       UEFI BOOT0002 : debian
       UEFI BOOT0003 : nvme0:1
       UEFI BOOT0004 : ubuntu
       UEFI BOOT0005 : nvme0:2
       UEFI BOOT0006 : usb0:2

mmcX:X, usbX:X and nvmeX:X are the entries of the removable media
having device path without FilePath. They are automatically created
by the bootmenu and BOOT#### and BootOrder variables are updated.
They are also managed by the bootmenu, if the usb device is removed from
the system at the next boot, the BOOT#### variable is removed by bootmenu
and BootOrder is also updated.

[*1] https://lore.kernel.org/u-boot/20220324135443.1571-11-masahisa.kojima@linaro.org/

Thanks,
Masahisa Kojima

>
> Thanks
> /Ilias
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes from original version:
> > - create new include file "efi_default_filename.h" to
> >   avoid conflict with config_distro_bootcmd.h
> > - modify the target pointer of efi_free_pool(), expand_media_path() should
> >   only free the pointer allocated by efi_dp_from_file() function.
> >
> >  include/config_distro_bootcmd.h | 14 +--------
> >  include/efi_default_filename.h  | 26 +++++++++++++++++
> >  lib/efi_loader/efi_bootmgr.c    | 50 ++++++++++++++++++++++++++++++++-
> >  3 files changed, 76 insertions(+), 14 deletions(-)
> >  create mode 100644 include/efi_default_filename.h
> >
> > diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> > index 2f90929178..ef2c9f330e 100644
> > --- a/include/config_distro_bootcmd.h
> > +++ b/include/config_distro_bootcmd.h
> > @@ -91,19 +91,7 @@
> >  #endif
> >
> >  #ifdef CONFIG_EFI_LOADER
> > -#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"
> > -#endif
> > +#include <efi_default_filename.h>
> >  #endif
> >
> >  #ifdef BOOTEFI_NAME
> > diff --git a/include/efi_default_filename.h b/include/efi_default_filename.h
> > new file mode 100644
> > index 0000000000..de030d2692
> > --- /dev/null
> > +++ b/include/efi_default_filename.h
> > @@ -0,0 +1,26 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Default boot file name when none is present in the FilePath.
> > + *
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +#ifndef _EFI_DEFAULT_FILENAME_H
> > +#define _EFI_DEFAULT_FILENAME_H
> > +
> > +#if defined(CONFIG_ARM64)
> > +#define BOOTEFI_NAME "BOOTAA64.EFI"
> > +#elif defined(CONFIG_ARM)
> > +#define BOOTEFI_NAME "BOOTARM.EFI"
> > +#elif defined(CONFIG_X86_64)
> > +#define BOOTEFI_NAME "BOOTX64.EFI"
> > +#elif defined(CONFIG_X86)
> > +#define BOOTEFI_NAME "BOOTIA32.EFI"
> > +#elif defined(CONFIG_ARCH_RV32I)
> > +#define BOOTEFI_NAME "BOOTRISCV32.EFI"
> > +#elif defined(CONFIG_ARCH_RV64I)
> > +#define BOOTEFI_NAME "BOOTRISCV64.EFI"
> > +#else
> > +#error Unsupported UEFI architecture
> > +#endif
> > +
> > +#endif
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 8c04ecbdc8..22a4302aac 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -11,6 +11,7 @@
> >  #include <charset.h>
> >  #include <log.h>
> >  #include <malloc.h>
> > +#include <efi_default_filename.h>
> >  #include <efi_loader.h>
> >  #include <efi_variable.h>
> >  #include <asm/unaligned.h>
> > @@ -30,6 +31,50 @@ static const struct efi_runtime_services *rs;
> >   * should do normal or recovery boot.
> >   */
> >
> > +/**
> > + * 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);
> > +                     efi_free_pool(dp);
> > +             } else {
> > +                     full_path = efi_dp_dup(device_path);
> > +             }
> > +     } else {
> > +             full_path = efi_dp_dup(device_path);
> > +     }
> > +
> > +     return full_path;
> > +}
> > +
> >  /**
> >   * try_load_entry() - try to load image for boot option
> >   *
> > @@ -68,13 +113,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.17.1
> >
Ilias Apalodimas March 31, 2022, 6:25 a.m. UTC | #3
Kojima-san,

On Thu, 31 Mar 2022 at 03:51, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Hi Ilias,
>
> On Thu, 31 Mar 2022 at 04:13, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Hello Akashi-san,
> >
> > On Thu, Mar 24, 2022 at 10:54:38PM +0900, Masahisa Kojima wrote:
> > > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >
> > > Under the current implementation, booting from removable 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
> >
> > Mark had some concerns wrt to this approach and from what I can tell this
> > hasn't changed in this revision [1]. Can we use boot_targets and generate
> > Boot#### with an empty FilePath as Mark suggested?  Or is the user expected to
> > select that somehow from the menu?
> >
> > [1] https://lore.kernel.org/u-boot/d3cac2e5b37f96b5@bloch.sibelius.xs4all.nl/
>
> My patch series tries to resolve the above issue to some extent
> by a different approach.
>
> The patch "[PATCH v4 10/11] bootmenu: add removable media entries" [*1]
> enumerates the all (removable) medias supporting
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL.
>
> Example bootmenu is as below.
>
>     *** U-Boot Boot Menu ***
>
>        UEFI BOOT0000 : mmc0:1
>        UEFI BOOT0001 : mmc0:2
>        UEFI BOOT0002 : debian
>        UEFI BOOT0003 : nvme0:1
>        UEFI BOOT0004 : ubuntu
>        UEFI BOOT0005 : nvme0:2
>        UEFI BOOT0006 : usb0:2
>
> mmcX:X, usbX:X and nvmeX:X are the entries of the removable media
> having device path without FilePath. They are automatically created
> by the bootmenu and BOOT#### and BootOrder variables are updated.
> They are also managed by the bootmenu, if the usb device is removed from
> the system at the next boot, the BOOT#### variable is removed by bootmenu
> and BootOrder is also updated.
>
> [*1] https://lore.kernel.org/u-boot/20220324135443.1571-11-masahisa.kojima@linaro.org/

Great thanks, that sounds reasonable. I'll go through the rest of the
series and let you know
/Ilias
>
> Thanks,
> Masahisa Kojima
>
> >
Heinrich Schuchardt April 2, 2022, 6:12 a.m. UTC | #4
On 3/24/22 14:54, Masahisa Kojima wrote:
> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>
> Under the current implementation, booting from removable 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.

At every boot you will have to delete autogenerated boot options and
create new ones according to the media which are present.

Your implementation does not offer any possibility to identify
autogenerated boot options.

On my laptop all autogenerated boot options use a VenMsg() device path

Boot0016* USB CD
VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,86701296aa5a7848b66cd49dd3ba6a55)
Boot0017* USB FDD
VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,6ff015a28830b543a8b8641009461e49)
Boot0018* NVMe0
VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,001c199932d94c4eae9aa0b6e98eb8a400)
Boot0019* ATA HDD0
VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,91af625956449f41a7b91f4f892ab0f600)
Boot001A* USB HDD
VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,33e821aaaf33bc4789bd419f88c50803)
Boot001B* PCI LAN
VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,78a84aaf2b2afc4ea79cf5cc8f3d3803)

while manual boot options use normal device paths

Boot0001* debian
HD(2,GPT,54e58b03-c1db-4c6b-afda-24340c3acda5,0x109000,0x32000)/File(\EFI\debian\shimx64.efi)
Boot0002* ubuntu
HD(2,GPT,54e58b03-c1db-4c6b-afda-24340c3acda5,0x109000,0x32000)/File(\EFI\ubuntu\shimx64.efi)

Please, provide a concept that can differentiate between autogenerated
and manually set boot options.

Best regards

Heinrich

>
> 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>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes from original version:
> - create new include file "efi_default_filename.h" to
>    avoid conflict with config_distro_bootcmd.h
> - modify the target pointer of efi_free_pool(), expand_media_path() should
>    only free the pointer allocated by efi_dp_from_file() function.
>
>   include/config_distro_bootcmd.h | 14 +--------
>   include/efi_default_filename.h  | 26 +++++++++++++++++
>   lib/efi_loader/efi_bootmgr.c    | 50 ++++++++++++++++++++++++++++++++-
>   3 files changed, 76 insertions(+), 14 deletions(-)
>   create mode 100644 include/efi_default_filename.h
>
> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> index 2f90929178..ef2c9f330e 100644
> --- a/include/config_distro_bootcmd.h
> +++ b/include/config_distro_bootcmd.h
> @@ -91,19 +91,7 @@
>   #endif
>
>   #ifdef CONFIG_EFI_LOADER
> -#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"
> -#endif
> +#include <efi_default_filename.h>
>   #endif
>
>   #ifdef BOOTEFI_NAME
> diff --git a/include/efi_default_filename.h b/include/efi_default_filename.h
> new file mode 100644
> index 0000000000..de030d2692
> --- /dev/null
> +++ b/include/efi_default_filename.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Default boot file name when none is present in the FilePath.
> + *
> + * Copyright (c) 2022, Linaro Limited
> + */
> +#ifndef _EFI_DEFAULT_FILENAME_H
> +#define _EFI_DEFAULT_FILENAME_H
> +
> +#if defined(CONFIG_ARM64)
> +#define BOOTEFI_NAME "BOOTAA64.EFI"
> +#elif defined(CONFIG_ARM)
> +#define BOOTEFI_NAME "BOOTARM.EFI"
> +#elif defined(CONFIG_X86_64)
> +#define BOOTEFI_NAME "BOOTX64.EFI"
> +#elif defined(CONFIG_X86)
> +#define BOOTEFI_NAME "BOOTIA32.EFI"
> +#elif defined(CONFIG_ARCH_RV32I)
> +#define BOOTEFI_NAME "BOOTRISCV32.EFI"
> +#elif defined(CONFIG_ARCH_RV64I)
> +#define BOOTEFI_NAME "BOOTRISCV64.EFI"
> +#else
> +#error Unsupported UEFI architecture
> +#endif
> +
> +#endif
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 8c04ecbdc8..22a4302aac 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -11,6 +11,7 @@
>   #include <charset.h>
>   #include <log.h>
>   #include <malloc.h>
> +#include <efi_default_filename.h>
>   #include <efi_loader.h>
>   #include <efi_variable.h>
>   #include <asm/unaligned.h>
> @@ -30,6 +31,50 @@ static const struct efi_runtime_services *rs;
>    * should do normal or recovery boot.
>    */
>
> +/**
> + * 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);
> +			efi_free_pool(dp);
> +		} else {
> +			full_path = efi_dp_dup(device_path);
> +		}
> +	} else {
> +		full_path = efi_dp_dup(device_path);
> +	}
> +
> +	return full_path;
> +}
> +
>   /**
>    * try_load_entry() - try to load image for boot option
>    *
> @@ -68,13 +113,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);
Masahisa Kojima April 4, 2022, 6:48 a.m. UTC | #5
On Sat, 2 Apr 2022 at 15:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 3/24/22 14:54, Masahisa Kojima wrote:
> > From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >
> > Under the current implementation, booting from removable 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.
>
> At every boot you will have to delete autogenerated boot options and
> create new ones according to the media which are present.
>
> Your implementation does not offer any possibility to identify
> autogenerated boot options.
>
> On my laptop all autogenerated boot options use a VenMsg() device path
>
> Boot0016* USB CD
> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,86701296aa5a7848b66cd49dd3ba6a55)
> Boot0017* USB FDD
> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,6ff015a28830b543a8b8641009461e49)
> Boot0018* NVMe0
> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,001c199932d94c4eae9aa0b6e98eb8a400)
> Boot0019* ATA HDD0
> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,91af625956449f41a7b91f4f892ab0f600)
> Boot001A* USB HDD
> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,33e821aaaf33bc4789bd419f88c50803)
> Boot001B* PCI LAN
> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,78a84aaf2b2afc4ea79cf5cc8f3d3803)
>
> while manual boot options use normal device paths
>
> Boot0001* debian
> HD(2,GPT,54e58b03-c1db-4c6b-afda-24340c3acda5,0x109000,0x32000)/File(\EFI\debian\shimx64.efi)
> Boot0002* ubuntu
> HD(2,GPT,54e58b03-c1db-4c6b-afda-24340c3acda5,0x109000,0x32000)/File(\EFI\ubuntu\shimx64.efi)
>
> Please, provide a concept that can differentiate between autogenerated
> and manually set boot options.

The patch "[PATCH v4 10/11] bootmenu: add removable media entries" [*1]
handles this auto generation and auto deletion.
 # Sorry, I should clearly describe this in the commit message.

u"bootmenu" string is stored in EFI_LOAD_OPTION.OptionalData of the
autogenerated Boot#### variable, to differentiate the boot options
between autogenerated and manually set.
 # In EDK2 implementation, a special GUID is used for this purpose and GUID
    is stored in EFI_LOAD_OPTION.OptionalData.
 # In U-Boot, lib/efi_loader/efi_load_option.c::efi_serialize_load_option()
    handles the EFI_LOAD_OPTION.OptionalData as u16 string, so I use
    "bootmenu" string.

The example is as below, Boot0003 is an auto generated boot option,
and Boot0004 is manually set.
==========
Boot0003:
attributes: A-- (0x00000001)
  label: virtio1:2
  file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(63293792-adf5-9325-b99f-4e0e455c1b1e,01)/HD(2,GPT,fb12642d-1b5a-4e31-b074-ba2d813bed71,0x100800,0x18fff)
  data:
    00000000: 62 00 6f 00 6f 00 74 00 6d 00 65 00 6e 00 75 00  b.o.o.t.m.e.n.u.
    00000010: 00 00                                            ..
Boot0004:
attributes: A-- (0x00000001)
  label: debian
  file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(63293792-adf5-9325-b99f-4e0e455c1b1e,00)/HD(1,GPT,c2475a57-2735-4744-9f01-67fefa1d06ae,0x800,0x100000)/EFI\debian\grubaa64.efi
  data:
==========

In "[PATCH v4 10/11]" [*1], auto generation is handled as follows.
 1) bootmenu enumerates the all devices having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
 2) bootmenu compares the device path enumerated in step#1 and
     existing boot option read from EFI variable
 3-a) if the enumerated device path in step#1 already exist in EFI variable,
        nothing to do.
 3-b) if the enumerated device path in step#1 does not exist in EFI variable,
        boot option is auto generated.
 3-c) If the boot option stored in EFI variable with "bootmenu" OptionalData
        does not appear in the device path enumerated in step#1,
        this boot option is treated as "invalid" and auto deleted.

[*1] https://lore.kernel.org/u-boot/20220324135443.1571-11-masahisa.kojima@linaro.org/

Thanks,
Masahisa Kojima

>
> Best regards
>
> Heinrich
>
> >
> > 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>
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> > Changes from original version:
> > - create new include file "efi_default_filename.h" to
> >    avoid conflict with config_distro_bootcmd.h
> > - modify the target pointer of efi_free_pool(), expand_media_path() should
> >    only free the pointer allocated by efi_dp_from_file() function.
> >
> >   include/config_distro_bootcmd.h | 14 +--------
> >   include/efi_default_filename.h  | 26 +++++++++++++++++
> >   lib/efi_loader/efi_bootmgr.c    | 50 ++++++++++++++++++++++++++++++++-
> >   3 files changed, 76 insertions(+), 14 deletions(-)
> >   create mode 100644 include/efi_default_filename.h
> >
> > diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
> > index 2f90929178..ef2c9f330e 100644
> > --- a/include/config_distro_bootcmd.h
> > +++ b/include/config_distro_bootcmd.h
> > @@ -91,19 +91,7 @@
> >   #endif
> >
> >   #ifdef CONFIG_EFI_LOADER
> > -#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"
> > -#endif
> > +#include <efi_default_filename.h>
> >   #endif
> >
> >   #ifdef BOOTEFI_NAME
> > diff --git a/include/efi_default_filename.h b/include/efi_default_filename.h
> > new file mode 100644
> > index 0000000000..de030d2692
> > --- /dev/null
> > +++ b/include/efi_default_filename.h
> > @@ -0,0 +1,26 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Default boot file name when none is present in the FilePath.
> > + *
> > + * Copyright (c) 2022, Linaro Limited
> > + */
> > +#ifndef _EFI_DEFAULT_FILENAME_H
> > +#define _EFI_DEFAULT_FILENAME_H
> > +
> > +#if defined(CONFIG_ARM64)
> > +#define BOOTEFI_NAME "BOOTAA64.EFI"
> > +#elif defined(CONFIG_ARM)
> > +#define BOOTEFI_NAME "BOOTARM.EFI"
> > +#elif defined(CONFIG_X86_64)
> > +#define BOOTEFI_NAME "BOOTX64.EFI"
> > +#elif defined(CONFIG_X86)
> > +#define BOOTEFI_NAME "BOOTIA32.EFI"
> > +#elif defined(CONFIG_ARCH_RV32I)
> > +#define BOOTEFI_NAME "BOOTRISCV32.EFI"
> > +#elif defined(CONFIG_ARCH_RV64I)
> > +#define BOOTEFI_NAME "BOOTRISCV64.EFI"
> > +#else
> > +#error Unsupported UEFI architecture
> > +#endif
> > +
> > +#endif
> > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> > index 8c04ecbdc8..22a4302aac 100644
> > --- a/lib/efi_loader/efi_bootmgr.c
> > +++ b/lib/efi_loader/efi_bootmgr.c
> > @@ -11,6 +11,7 @@
> >   #include <charset.h>
> >   #include <log.h>
> >   #include <malloc.h>
> > +#include <efi_default_filename.h>
> >   #include <efi_loader.h>
> >   #include <efi_variable.h>
> >   #include <asm/unaligned.h>
> > @@ -30,6 +31,50 @@ static const struct efi_runtime_services *rs;
> >    * should do normal or recovery boot.
> >    */
> >
> > +/**
> > + * 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);
> > +                     efi_free_pool(dp);
> > +             } else {
> > +                     full_path = efi_dp_dup(device_path);
> > +             }
> > +     } else {
> > +             full_path = efi_dp_dup(device_path);
> > +     }
> > +
> > +     return full_path;
> > +}
> > +
> >   /**
> >    * try_load_entry() - try to load image for boot option
> >    *
> > @@ -68,13 +113,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);
>
Heinrich Schuchardt April 4, 2022, 9:54 p.m. UTC | #6
On 4/4/22 08:48, Masahisa Kojima wrote:
> On Sat, 2 Apr 2022 at 15:12, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 3/24/22 14:54, Masahisa Kojima wrote:
>>> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>
>>> Under the current implementation, booting from removable 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.
>>
>> At every boot you will have to delete autogenerated boot options and
>> create new ones according to the media which are present.
>>
>> Your implementation does not offer any possibility to identify
>> autogenerated boot options.
>>
>> On my laptop all autogenerated boot options use a VenMsg() device path
>>
>> Boot0016* USB CD
>> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,86701296aa5a7848b66cd49dd3ba6a55)
>> Boot0017* USB FDD
>> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,6ff015a28830b543a8b8641009461e49)
>> Boot0018* NVMe0
>> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,001c199932d94c4eae9aa0b6e98eb8a400)
>> Boot0019* ATA HDD0
>> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,91af625956449f41a7b91f4f892ab0f600)
>> Boot001A* USB HDD
>> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,33e821aaaf33bc4789bd419f88c50803)
>> Boot001B* PCI LAN
>> VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,78a84aaf2b2afc4ea79cf5cc8f3d3803)
>>
>> while manual boot options use normal device paths
>>
>> Boot0001* debian
>> HD(2,GPT,54e58b03-c1db-4c6b-afda-24340c3acda5,0x109000,0x32000)/File(\EFI\debian\shimx64.efi)
>> Boot0002* ubuntu
>> HD(2,GPT,54e58b03-c1db-4c6b-afda-24340c3acda5,0x109000,0x32000)/File(\EFI\ubuntu\shimx64.efi)
>>
>> Please, provide a concept that can differentiate between autogenerated
>> and manually set boot options.
>
> The patch "[PATCH v4 10/11] bootmenu: add removable media entries" [*1]
> handles this auto generation and auto deletion.
>   # Sorry, I should clearly describe this in the commit message.
>
> u"bootmenu" string is stored in EFI_LOAD_OPTION.OptionalData of the
> autogenerated Boot#### variable, to differentiate the boot options
> between autogenerated and manually set.
>   # In EDK2 implementation, a special GUID is used for this purpose and GUID
>      is stored in EFI_LOAD_OPTION.OptionalData.

EDK II uses GUID mBmAutoCreateBootOptionGuid
in MdeModulePkg/Library/UefiBootManagerLib/BmBoot.c.

The chance of a collision for a GUID is much less then when using a
string. Hence I would prefer a GUID based solution.

An alternative to storing the GUID in the optional data is in the device
path. This is what I see on my laptop:

Boot001A* USB HDD
VenMsg(bc7838d2-0f82-4d60-8316-c068ee79d25b,
33e821aaaf33bc4783bd419f88c70803)

But using a GUID in the optional data is good enough.

Best regards

Heinrich

>   # In U-Boot, lib/efi_loader/efi_load_option.c::efi_serialize_load_option()
>      handles the EFI_LOAD_OPTION.OptionalData as u16 string, so I use
>      "bootmenu" string.
>
> The example is as below, Boot0003 is an auto generated boot option,
> and Boot0004 is manually set.
> ==========
> Boot0003:
> attributes: A-- (0x00000001)
>    label: virtio1:2
>    file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(63293792-adf5-9325-b99f-4e0e455c1b1e,01)/HD(2,GPT,fb12642d-1b5a-4e31-b074-ba2d813bed71,0x100800,0x18fff)
>    data:
>      00000000: 62 00 6f 00 6f 00 74 00 6d 00 65 00 6e 00 75 00  b.o.o.t.m.e.n.u.
>      00000010: 00 00                                            ..
> Boot0004:
> attributes: A-- (0x00000001)
>    label: debian
>    file_path: /VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b)/VenHw(63293792-adf5-9325-b99f-4e0e455c1b1e,00)/HD(1,GPT,c2475a57-2735-4744-9f01-67fefa1d06ae,0x800,0x100000)/EFI\debian\grubaa64.efi
>    data:
> ==========
>
> In "[PATCH v4 10/11]" [*1], auto generation is handled as follows.
>   1) bootmenu enumerates the all devices having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL
>   2) bootmenu compares the device path enumerated in step#1 and
>       existing boot option read from EFI variable
>   3-a) if the enumerated device path in step#1 already exist in EFI variable,
>          nothing to do.
>   3-b) if the enumerated device path in step#1 does not exist in EFI variable,
>          boot option is auto generated.
>   3-c) If the boot option stored in EFI variable with "bootmenu" OptionalData
>          does not appear in the device path enumerated in step#1,
>          this boot option is treated as "invalid" and auto deleted.
>
> [*1] https://lore.kernel.org/u-boot/20220324135443.1571-11-masahisa.kojima@linaro.org/
>
> Thanks,
> Masahisa Kojima
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> 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>
>>> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
>>> ---
>>> Changes from original version:
>>> - create new include file "efi_default_filename.h" to
>>>     avoid conflict with config_distro_bootcmd.h
>>> - modify the target pointer of efi_free_pool(), expand_media_path() should
>>>     only free the pointer allocated by efi_dp_from_file() function.
>>>
>>>    include/config_distro_bootcmd.h | 14 +--------
>>>    include/efi_default_filename.h  | 26 +++++++++++++++++
>>>    lib/efi_loader/efi_bootmgr.c    | 50 ++++++++++++++++++++++++++++++++-
>>>    3 files changed, 76 insertions(+), 14 deletions(-)
>>>    create mode 100644 include/efi_default_filename.h
>>>
>>> diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
>>> index 2f90929178..ef2c9f330e 100644
>>> --- a/include/config_distro_bootcmd.h
>>> +++ b/include/config_distro_bootcmd.h
>>> @@ -91,19 +91,7 @@
>>>    #endif
>>>
>>>    #ifdef CONFIG_EFI_LOADER
>>> -#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"
>>> -#endif
>>> +#include <efi_default_filename.h>
>>>    #endif
>>>
>>>    #ifdef BOOTEFI_NAME
>>> diff --git a/include/efi_default_filename.h b/include/efi_default_filename.h
>>> new file mode 100644
>>> index 0000000000..de030d2692
>>> --- /dev/null
>>> +++ b/include/efi_default_filename.h
>>> @@ -0,0 +1,26 @@
>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>> +/*
>>> + * Default boot file name when none is present in the FilePath.
>>> + *
>>> + * Copyright (c) 2022, Linaro Limited
>>> + */
>>> +#ifndef _EFI_DEFAULT_FILENAME_H
>>> +#define _EFI_DEFAULT_FILENAME_H
>>> +
>>> +#if defined(CONFIG_ARM64)
>>> +#define BOOTEFI_NAME "BOOTAA64.EFI"
>>> +#elif defined(CONFIG_ARM)
>>> +#define BOOTEFI_NAME "BOOTARM.EFI"
>>> +#elif defined(CONFIG_X86_64)
>>> +#define BOOTEFI_NAME "BOOTX64.EFI"
>>> +#elif defined(CONFIG_X86)
>>> +#define BOOTEFI_NAME "BOOTIA32.EFI"
>>> +#elif defined(CONFIG_ARCH_RV32I)
>>> +#define BOOTEFI_NAME "BOOTRISCV32.EFI"
>>> +#elif defined(CONFIG_ARCH_RV64I)
>>> +#define BOOTEFI_NAME "BOOTRISCV64.EFI"
>>> +#else
>>> +#error Unsupported UEFI architecture
>>> +#endif
>>> +
>>> +#endif
>>> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
>>> index 8c04ecbdc8..22a4302aac 100644
>>> --- a/lib/efi_loader/efi_bootmgr.c
>>> +++ b/lib/efi_loader/efi_bootmgr.c
>>> @@ -11,6 +11,7 @@
>>>    #include <charset.h>
>>>    #include <log.h>
>>>    #include <malloc.h>
>>> +#include <efi_default_filename.h>
>>>    #include <efi_loader.h>
>>>    #include <efi_variable.h>
>>>    #include <asm/unaligned.h>
>>> @@ -30,6 +31,50 @@ static const struct efi_runtime_services *rs;
>>>     * should do normal or recovery boot.
>>>     */
>>>
>>> +/**
>>> + * 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);
>>> +                     efi_free_pool(dp);
>>> +             } else {
>>> +                     full_path = efi_dp_dup(device_path);
>>> +             }
>>> +     } else {
>>> +             full_path = efi_dp_dup(device_path);
>>> +     }
>>> +
>>> +     return full_path;
>>> +}
>>> +
>>>    /**
>>>     * try_load_entry() - try to load image for boot option
>>>     *
>>> @@ -68,13 +113,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);
>>
diff mbox series

Patch

diff --git a/include/config_distro_bootcmd.h b/include/config_distro_bootcmd.h
index 2f90929178..ef2c9f330e 100644
--- a/include/config_distro_bootcmd.h
+++ b/include/config_distro_bootcmd.h
@@ -91,19 +91,7 @@ 
 #endif
 
 #ifdef CONFIG_EFI_LOADER
-#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"
-#endif
+#include <efi_default_filename.h>
 #endif
 
 #ifdef BOOTEFI_NAME
diff --git a/include/efi_default_filename.h b/include/efi_default_filename.h
new file mode 100644
index 0000000000..de030d2692
--- /dev/null
+++ b/include/efi_default_filename.h
@@ -0,0 +1,26 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Default boot file name when none is present in the FilePath.
+ *
+ * Copyright (c) 2022, Linaro Limited
+ */
+#ifndef _EFI_DEFAULT_FILENAME_H
+#define _EFI_DEFAULT_FILENAME_H
+
+#if defined(CONFIG_ARM64)
+#define BOOTEFI_NAME "BOOTAA64.EFI"
+#elif defined(CONFIG_ARM)
+#define BOOTEFI_NAME "BOOTARM.EFI"
+#elif defined(CONFIG_X86_64)
+#define BOOTEFI_NAME "BOOTX64.EFI"
+#elif defined(CONFIG_X86)
+#define BOOTEFI_NAME "BOOTIA32.EFI"
+#elif defined(CONFIG_ARCH_RV32I)
+#define BOOTEFI_NAME "BOOTRISCV32.EFI"
+#elif defined(CONFIG_ARCH_RV64I)
+#define BOOTEFI_NAME "BOOTRISCV64.EFI"
+#else
+#error Unsupported UEFI architecture
+#endif
+
+#endif
diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 8c04ecbdc8..22a4302aac 100644
--- a/lib/efi_loader/efi_bootmgr.c
+++ b/lib/efi_loader/efi_bootmgr.c
@@ -11,6 +11,7 @@ 
 #include <charset.h>
 #include <log.h>
 #include <malloc.h>
+#include <efi_default_filename.h>
 #include <efi_loader.h>
 #include <efi_variable.h>
 #include <asm/unaligned.h>
@@ -30,6 +31,50 @@  static const struct efi_runtime_services *rs;
  * should do normal or recovery boot.
  */
 
+/**
+ * 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);
+			efi_free_pool(dp);
+		} else {
+			full_path = efi_dp_dup(device_path);
+		}
+	} else {
+		full_path = efi_dp_dup(device_path);
+	}
+
+	return full_path;
+}
+
 /**
  * try_load_entry() - try to load image for boot option
  *
@@ -68,13 +113,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);