Re: [PATCH v2 4/9] efi_loader: support booting via short-form device-path

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

On Sat, Mar 19, 2022 at 10:11:43AM +0100, Heinrich Schuchardt wrote:
> The boot manager must support loading from boot options using a short-form
> device-path, e.g. one where the first element is a hard drive media path.
> 
> See '3.1.2 Load Options Processing' in UEFI specification version 2.9.

Thank you for adding this feature.
Just reminder; a hard drive media path is not the only case for allowing
short-form paths. In particular,  booting from a short-form path starting
with a *file path* device path is also, in my opinion, valuable.

So we should have some description, in the commit message or the comment,
about this sort of limitation or TODO in handling short-form paths even
after this commit.

> Fixes: 0e074d12393b ("efi_loader: carve out efi_load_image_from_file()")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	path correct remaining paths to LOAD_FILE(2) protocol
> ---
>  lib/efi_loader/efi_boottime.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index a7bc371f54..5bcb8253ed 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1940,7 +1940,7 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
>  {
>  	efi_handle_t device;
>  	efi_status_t ret;
> -	struct efi_device_path *dp;
> +	struct efi_device_path *dp, *rem;
>  	struct efi_load_file_protocol *load_file_protocol = NULL;
>  	efi_uintn_t buffer_size;
>  	uint64_t addr, pages;
> @@ -1951,18 +1951,18 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
>  	*size = 0;
>  
>  	dp = file_path;
> -	ret = EFI_CALL(efi_locate_device_path(
> -		       &efi_simple_file_system_protocol_guid, &dp, &device));
> +	device = efi_dp_find_obj(dp, NULL, &rem);
> +	ret = efi_search_protocol(device, &efi_simple_file_system_protocol_guid,
> +				  NULL);

I think that you intended to use your *new* efi_dp_find_obj() in patch#3 here.

-Takahiro Akashi


>  	if (ret == EFI_SUCCESS)
>  		return efi_load_image_from_file(file_path, buffer, size);
>  
> -	ret = EFI_CALL(efi_locate_device_path(
> -		       &efi_guid_load_file_protocol, &dp, &device));
> +	ret = efi_search_protocol(device, &efi_guid_load_file_protocol, NULL);
>  	if (ret == EFI_SUCCESS) {
>  		guid = &efi_guid_load_file_protocol;
>  	} else if (!boot_policy) {
>  		guid = &efi_guid_load_file2_protocol;
> -		ret = EFI_CALL(efi_locate_device_path(guid, &dp, &device));
> +		ret = efi_search_protocol(device, guid, NULL);
>  	}
>  	if (ret != EFI_SUCCESS)
>  		return EFI_NOT_FOUND;
> @@ -1971,9 +1971,9 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
>  	if (ret != EFI_SUCCESS)
>  		return EFI_NOT_FOUND;
>  	buffer_size = 0;
> -	ret = load_file_protocol->load_file(load_file_protocol, dp,
> -					    boot_policy, &buffer_size,
> -					    NULL);
> +	ret = EFI_CALL(load_file_protocol->load_file(
> +					load_file_protocol, rem, boot_policy,
> +					&buffer_size, NULL));
>  	if (ret != EFI_BUFFER_TOO_SMALL)
>  		goto out;
>  	pages = efi_size_in_pages(buffer_size);
> @@ -1984,7 +1984,7 @@ efi_status_t efi_load_image_from_path(bool boot_policy,
>  		goto out;
>  	}
>  	ret = EFI_CALL(load_file_protocol->load_file(
> -					load_file_protocol, dp, boot_policy,
> +					load_file_protocol, rem, boot_policy,
>  					&buffer_size, (void *)(uintptr_t)addr));
>  	if (ret != EFI_SUCCESS)
>  		efi_free_pages(addr, pages);
> -- 
> 2.34.1
>