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

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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